Skip to content

Conversation

@arneboe
Copy link

@arneboe arneboe commented Oct 9, 2018

The mainfest defines sisl as optional but base-types does not compile without sisl. This PR fixes that issue.

This reduces the number of dependencies that we need to install when using base-types outside of rock. In our use case we are dependent on base types but not on Spline/Trajectory, thus we do not need the SISL dependency.

@@ -1,5 +1,22 @@

find_package(SISL)
Copy link
Member

Choose a reason for hiding this comment

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

I would rather that this is controlled by an explicit option (i.e. WITH_SISL), to avoid having people getting an unexpectedly crippled base/types.

Copy link
Member

Choose a reason for hiding this comment

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

Adding an option would be fine for me.

However, I don't understand how this should interact with <depend package="external/sisl" optional="1" /> from package.xml. If the dependency is not found, would it still be mandatory to manually set the option?

Perhaps we could always install Spline.hpp and Trajectory.hpp, but have it generate an #error, if SISL was not found?

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps we could always install Spline.hpp and Trajectory.hpp, but have it generate an #error, if SISL was not found?

This shifts the error to the compile phase instead of the configuration phase where it should occur.

In general I am fine with an option but it conflicts with having optional=1 in the manifest.

Copy link
Member

Choose a reason for hiding this comment

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

In general I am fine with an option but it conflicts with having optional=1 in the manifest.

Not necessarily. Autoproj will have to set the flag if sisl is not excluded, which would be done with

unless Autoproj.manifest.excluded?('external/sisl')

end

Within the package definition block in rock.core's 02stable.autobuild

Note that the package_enabled? call is actually deprecated since Autoproj v2. To facilitate the transition between v1 and v2, I disabled the deprecation warnings by default and then forgot I did :(

@doudou
Copy link
Member

doudou commented Apr 26, 2019

Closing this now that #134 has been merged.

@doudou doudou closed this Apr 26, 2019
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.

3 participants