search

ament / ament_index

Apache License 2.0
13 stars 27 forks source link

Refactor paths to use rcpputils filesystem helper #46

Closed piraka9011 closed 4 years ago

piraka9011 commented 4 years ago

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

piraka9011 commented 4 years ago

@wjwwood nits resolved!

wjwwood commented 4 years ago

CI:

wjwwood commented 4 years ago

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]
zmichaels11 commented 4 years ago

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

wjwwood commented 4 years ago

Yeah, feel free to do that.

piraka9011 commented 4 years ago
piraka9011 commented 4 years ago

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

wjwwood commented 4 years ago

Should I re-run CI?

Not yet, I'm looking into it.

wjwwood commented 4 years ago

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

wjwwood commented 4 years ago

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

piraka9011 commented 4 years ago

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 commented 4 years ago

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

clalancette commented 4 years ago

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.

piraka9011 commented 4 years ago

@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.