Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor paths to use rcpputils filesystem helper #46

Merged
merged 4 commits into from
Feb 11, 2020
Merged

Refactor paths to use rcpputils filesystem helper #46

merged 4 commits into from
Feb 11, 2020

Conversation

piraka9011
Copy link
Contributor

The purpose of this refactor is to provide proper path support for both Linux and Windows systems. The previous implementation hard codes forward slashes which are not the convention on Windows systems. Using the filesystem helper in rcpputils resolves this.
Using rcpputils pulls in the dependency on rcutils for this package as well.

I didn't change on the return signatures for the methods to avoid breaking any previous workflows.

Resolves #45

Signed-off-by: Anas Abou Allaban allabana@amazon.com

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than some trivial feedback on include statement order, lgtm.

I don't think there's any issue with adding rcpputils as a dependency of ament_index_cpp.

ament_index_cpp/src/get_resource.cpp Show resolved Hide resolved
ament_index_cpp/src/get_resources.cpp Show resolved Hide resolved
ament_index_cpp/src/has_resource.cpp Outdated Show resolved Hide resolved
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

@wjwwood nits resolved!

Copy link
Contributor

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@wjwwood
Copy link
Contributor

wjwwood commented Feb 7, 2020

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

@wjwwood
Copy link
Contributor

wjwwood commented Feb 7, 2020

Looks like a compiler error on Windows:

11:58:21 [7.265s]   has_resource.cpp
11:58:21 [7.984s] C:\J\workspace\ci_windows\ws\src\ament\ament_index\ament_index_cpp\src\get_resources.cpp(75,58): error C2440: 'initializing': cannot convert from 'rcpputils::fs::path' to 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>' [C:\J\workspace\ci_windows\ws\build\ament_index_cpp\ament_index_cpp.vcxproj]
11:58:21 [8.578s] C:\J\workspace\ci_windows\ws\src\ament\ament_index\ament_index_cpp\src\get_resources.cpp(75,58): message : No constructor could take the source type, or constructor overload resolution was ambiguous [C:\J\workspace\ci_windows\ws\build\ament_index_cpp\ament_index_cpp.vcxproj]

ament_index_cpp/src/get_resources.cpp Outdated Show resolved Hide resolved
@zmichaels11
Copy link

Can I stop the rest of the CI since windows failed?

@wjwwood
Copy link
Contributor

wjwwood commented Feb 7, 2020

Yeah, feel free to do that.

Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
@piraka9011
Copy link
Contributor Author

piraka9011 commented Feb 7, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
@piraka9011
Copy link
Contributor Author

@wjwwood it looks like the builds are unstable due to an unrelated issue (flake8/copyright). Should I re-run CI?

@wjwwood
Copy link
Contributor

wjwwood commented Feb 10, 2020

Should I re-run CI?

Not yet, I'm looking into it.

@wjwwood
Copy link
Contributor

wjwwood commented Feb 10, 2020

@piraka9011 can you rerun? I think we've reverted the offending change.

@wjwwood
Copy link
Contributor

wjwwood commented Feb 10, 2020

Re-running my CI from several comments above (not sure if the settings are similar to @piraka9011's):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

@piraka9011
Copy link
Contributor Author

Re-running my CI from several comments above (not sure if the settings are similar to @piraka9011's):

They are the same. I wanted to run the CI at the end of the day since it's a lot of packages and I didn't want to hog the queue.

@wjwwood
Copy link
Contributor

wjwwood commented Feb 10, 2020

Ok, no worries, that was thoughtful of you, but I already started them, so we'll just let them run.

@wjwwood wjwwood merged commit 5954265 into ament:master Feb 11, 2020
@clalancette
Copy link
Contributor

For reasons I don't fully understand, it looks like this PR broke the packaging jobs on Linux. For instance, if you download any packaging job from https://ci.ros2.org/view/packaging/job/packaging_linux/ since Feb 10th, and try to compile https://github.com/ros-perception/image_common/tree/ros2 and https://github.com/ros-perception/image_pipeline/tree/ros2 after sourcing it, you'll get:

CMake Error in CMakeLists.txt:                       
  Imported target "ament_index_cpp::ament_index_cpp" includes non-existent
  path

    "/home/jenkins-agent/workspace/packaging_linux/ws/install/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

when trying to build image_proc. I tried reverting just this change in https://ci.ros2.org/job/ci_packaging_linux/296/ , and things went back to working. @piraka9011 can you look into it? In the meantime, I'm going to propose a revert so we can get the packaging jobs working again.

clalancette added a commit that referenced this pull request Feb 21, 2020
@clalancette clalancette mentioned this pull request Feb 21, 2020
clalancette added a commit that referenced this pull request Feb 21, 2020
This reverts commit 5954265.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
wjwwood pushed a commit that referenced this pull request Feb 21, 2020
* Revert "Address clang-tidy warnings (#47)"

This reverts commit 602d4b1.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

* Revert "Refactor paths to use rcpputils filesystem helper (#46)"

This reverts commit 5954265.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@piraka9011
Copy link
Contributor Author

@clalancette Sure, I'm not sure why this it occurred and why CI didn't catch it but I can look into it when I get the chance.

Blast545 pushed a commit that referenced this pull request May 6, 2020
* Revert "Address clang-tidy warnings (#47)"

This reverts commit 602d4b1.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

* Revert "Refactor paths to use rcpputils filesystem helper (#46)"

This reverts commit 5954265.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Jorge Perez <jjperez@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_package_share_directory does not return a conventional Windows directory
4 participants