Add a plugin mechanism to resource_retriever #103

mjcarroll · 2024-08-16T19:54:23Z

The idea behind this is to make resource_retriever pluggable. That is that downstream users of the library can choose the mechanisms that they want to back the calls to "get".

This will allow for things like retrieving over ROS services or parameters.

wjwwood

This makes sense to me, it's the best way to avoid resource retriever from being dependent on large things like ROS, while also keeping it somewhat easy to integrate into other non-ROS projects for relatively simple things like file:/// and package:/// urls.

resource_retriever/include/resource_retriever/plugins/curl_retriever.hpp

resource_retriever/include/resource_retriever/plugins/filesystem_retriever.hpp

resource_retriever/src/plugins/curl_retriever.cpp

resource_retriever/include/resource_retriever/plugins/curl_retriever.hpp

resource_retriever/src/plugins/curl_retriever.cpp

resource_retriever/include/resource_retriever/memory_resource.hpp

resource_retriever/src/plugins/curl_retriever.cpp

resource_retriever/src/plugins/filesystem_retriever.cpp

resource_retriever/src/plugins/retriever_plugin.cpp

resource_retriever/src/retriever.cpp

resource_retriever/test/test.cpp

wjwwood · 2024-12-14T00:25:47Z

I fixed up the DCO and addressed review comments in 8a30432, @clalancette could you have another quick look?

resource_retriever/include/resource_retriever/retriever.hpp

out-of-date

Yadunund

LGTM with a comment on copyright headers.

resource_retriever/include/resource_retriever/exception.hpp

resource_retriever/include/resource_retriever/memory_resource.hpp

resource_retriever/include/resource_retriever/retriever.hpp

resource_retriever/src/plugins/curl_retriever.cpp

resource_retriever/src/plugins/filesystem_retriever.cpp

ahcorde · 2025-02-19T08:58:56Z

Pulls: #103
Gist: https://gist.githubusercontent.com/ahcorde/9e7444d1b56419e258ff10755d70746b/raw/86df9c511efc6fef5cd9d84105d21785973d9df4/ros2.repos
BUILD args: --packages-above-and-dependencies resource_retriever
TEST args: --packages-above resource_retriever
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15218

Linux
Linux-aarch64
Linux-rhel
Windows

ahcorde · 2025-02-19T09:38:08Z

Pulls: #103, ros2/rviz#1262
Gist: https://gist.githubusercontent.com/ahcorde/13502377fe7128ba0a2d910a7c9dfd91/raw/2b32e8eeb4f5c28e7b0afbbdb6ea9f15771e6234/ros2.repos
BUILD args: --packages-above-and-dependencies resource_retriever
TEST args: --packages-above resource_retriever
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15221

Linux
Linux-aarch64
Linux-rhel
Windows

resource_retriever/src/plugins/filesystem_retriever.cpp

mjcarroll · 2025-03-04T23:37:57Z

Pulls: #103
Gist: https://gist.githubusercontent.com/mjcarroll/cc5568432e45f4a942cb2b519c381f26/raw/86df9c511efc6fef5cd9d84105d21785973d9df4/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15285

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

Signed-off-by: William Woodall <[email protected]>

Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

Signed-off-by: William Woodall <[email protected]>

…and deprecate get() Signed-off-by: William Woodall <[email protected]>

Signed-off-by: William Woodall <[email protected]>

Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2025-03-05T01:18:46Z

I fixed the API breaks in 67b9130 and a7607e6. Should be good to re-run CI.

ahcorde · 2025-03-05T13:57:13Z

Linux
Linux-aarch64
Linux-rhel
Windows

wjwwood · 2025-03-07T19:28:27Z

Looks like ci was interrupted?

ahcorde · 2025-03-07T21:32:19Z

Linux
Linux-aarch64
Linux-rhel
Windows

wjwwood · 2025-03-11T19:46:34Z

Linux

Linux-aarch64

Linux-rhel

Windows

I think this CI was with the rviz changes too, which I still need to update. I'm going to re-run what @mjcarroll did before which was testing that these changes only introduced deprecation warnings (without the rviz changes):

Linux
Linux-aarch64
Linux-rhel
Windows (edit: updated to job that was restarted)

wjwwood · 2025-03-11T22:45:29Z

I fixed up the rviz changes now too in ros2/rviz2@9c3ea584. So I'll rerun your CI @ahcorde:

Linux
Linux-aarch64
Linux-rhel
Windows

wjwwood · 2025-03-12T16:50:46Z

Weirdly gcc 11.5 on RHEL doesn't like two attributes on a struct, Windows and whatever we're using on Ubuntu don't mind, see this godbolt with 11.4 (closest I could find on there):

https://godbolt.org/z/r8obexM18

I guess I'll just not deprecate it on RHEL? I'm not sure what to do here. @cottsay FYI in case this comes up anywhere else.

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2025-03-12T17:37:36Z

I managed this workaround (I think): 5e78208

New CI (just up to resource_retriever and no rviz changes because I need to investigate rviz test failures first):

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2025-03-12T18:31:22Z

7b55419 should fix the issues in rviz.

CI without rviz changes:

Linux
Linux-aarch64
Linux-rhel
Windows

CI with rviz changes (ros2/rviz#1262):

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2025-03-13T00:29:33Z

I restored the old exception throwing behavior in 6f5eb98, which some of the rviz tests relied on, but we should really preserve if we can.

wjwwood · 2025-03-13T00:30:17Z

Another round of CI without changes to rviz:

Linux
Linux-aarch64
Linux-rhel
Windows

edit: Canceled these because I found a new test failure.

wjwwood · 2025-03-13T01:17:14Z

Yet another round of CI without rviz changes, this time after running the tests locally 🤦 :

Linux
Linux-aarch64
Linux-rhel
Windows

Signed-off-by: William Woodall <[email protected]>

wjwwood · 2025-03-13T16:05:08Z

I forgot to push... man I'm on the struggle 🚌 , CI without rviz changes again:

Linux
Linux-aarch64
Linux-rhel
Windows

mjcarroll requested review from sloretz and wjwwood August 16, 2024 19:54

mjcarroll self-assigned this Aug 16, 2024

wjwwood approved these changes Aug 17, 2024

View reviewed changes

ahcorde previously requested changes Aug 19, 2024

View reviewed changes

ahcorde added the ros2 label Aug 20, 2024

Yadunund reviewed Aug 21, 2024

View reviewed changes

resource_retriever/include/resource_retriever/memory_resource.hpp Outdated Show resolved Hide resolved

mjcarroll marked this pull request as ready for review December 3, 2024 16:07

clalancette requested changes Dec 3, 2024

View reviewed changes

wjwwood force-pushed the mjcarroll/plugins branch from 4ffb1bc to 8a30432 Compare December 14, 2024 00:25

wjwwood requested review from ahcorde and clalancette December 14, 2024 00:25

wjwwood force-pushed the mjcarroll/plugins branch from 8a30432 to 8e0fae7 Compare December 17, 2024 12:57

clalancette previously requested changes Dec 18, 2024

View reviewed changes

resource_retriever/include/resource_retriever/retriever.hpp Outdated Show resolved Hide resolved

resource_retriever/include/resource_retriever/retriever.hpp Outdated Show resolved Hide resolved

wjwwood requested a review from clalancette February 14, 2025 22:00

Yadunund approved these changes Feb 18, 2025

View reviewed changes

resource_retriever/include/resource_retriever/exception.hpp Show resolved Hide resolved

ahcorde requested changes Feb 18, 2025

View reviewed changes

wjwwood requested a review from ahcorde February 18, 2025 23:05

wjwwood mentioned this pull request Feb 19, 2025

Work in progress using the new resource retriever apis ros2/rviz#1262

Merged

ahcorde approved these changes Feb 19, 2025

View reviewed changes

ahcorde requested changes Feb 19, 2025

View reviewed changes

resource_retriever/src/plugins/filesystem_retriever.cpp Outdated Show resolved Hide resolved

mjcarroll requested a review from ahcorde March 4, 2025 23:27

mjcarroll added 2 commits March 4, 2025 17:17

Add a plugin mechanism to resource_retriever

f837b72

Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

Update memory resource

c021e5a

Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

wjwwood and others added 10 commits March 4, 2025 17:17

avoid variable shadowing

28bdd38

Signed-off-by: William Woodall <[email protected]>

Lint

a01169e

Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

address review comments and change error handling for libcurl

1b21cc9

Signed-off-by: William Woodall <[email protected]>

fixup

9b2279a

Signed-off-by: William Woodall <[email protected]>

get() -> get_shared(), MemoryResourcePtr -> MemoryResourceSharedPtr, …

d752ce8

…and deprecate get() Signed-off-by: William Woodall <[email protected]>

copyright update

e240906

Signed-off-by: William Woodall <[email protected]>

address review feedback

5e62db4

Signed-off-by: William Woodall <[email protected]>

Update resource_retriever/src/plugins/filesystem_retriever.cpp

4ed3eb7

Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Michael Carroll <[email protected]> Signed-off-by: William Woodall <[email protected]>

fixup broken change to MemoryResource, deprecate for Resource

67b9130

Signed-off-by: William Woodall <[email protected]>

add missing include to prevent API break of resource_retriever

a7607e6

Signed-off-by: William Woodall <[email protected]>

wjwwood force-pushed the mjcarroll/plugins branch from 7050540 to a7607e6 Compare March 5, 2025 01:18

avoid mixing [[deprecated]] with other __attribute__ statements for RHEL

5e78208

Signed-off-by: William Woodall <[email protected]>

handle not found resource case again in get() method

7b55419

Signed-off-by: William Woodall <[email protected]>

restore exception behavior for get()

6f5eb98

Signed-off-by: William Woodall <[email protected]>

fix test for invalid files

cf326a1

Signed-off-by: William Woodall <[email protected]>

mjcarroll merged commit cc80ce3 into rolling Apr 3, 2025
2 checks passed

mjcarroll deleted the mjcarroll/plugins branch April 3, 2025 16:34

Add a plugin mechanism to resource_retriever #103

Add a plugin mechanism to resource_retriever #103

Uh oh!

Conversation

mjcarroll commented Aug 16, 2024

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wjwwood commented Dec 14, 2024

Uh oh!

Uh oh!

Uh oh!

Yadunund left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Feb 19, 2025

Uh oh!

ahcorde commented Feb 19, 2025

Uh oh!

Uh oh!

mjcarroll commented Mar 4, 2025

Uh oh!

wjwwood commented Mar 5, 2025

Uh oh!

ahcorde commented Mar 5, 2025

Uh oh!

wjwwood commented Mar 7, 2025

Uh oh!

ahcorde commented Mar 7, 2025

Uh oh!

wjwwood commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Mar 11, 2025

Uh oh!

wjwwood commented Mar 12, 2025

Uh oh!

wjwwood commented Mar 12, 2025

Uh oh!

wjwwood commented Mar 12, 2025

Uh oh!

wjwwood commented Mar 13, 2025

Uh oh!

wjwwood commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Mar 13, 2025

Uh oh!

wjwwood commented Mar 13, 2025 • edited by ahcorde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

wjwwood commented Mar 11, 2025 •

edited

Loading

wjwwood commented Mar 13, 2025 •

edited

Loading

wjwwood commented Mar 13, 2025 •

edited by ahcorde

Loading