Skip to content

Conversation

2maz
Copy link
Member

@2maz 2maz commented Apr 4, 2019

@saarnold

should be applied in combination with: rock-core/tools-service_discovery#9
base-orogen-types also requires adaption which is WIP: rock-core/base-orogen-types#29

@2maz 2maz force-pushed the fix_sisl_optional branch from 252db9c to 4c2964c Compare April 4, 2019 17:03
@doudou
Copy link
Member

doudou commented Apr 4, 2019

Please don't make it dependent on the presence or not of SISL, which will only cause pain for people that for whatever reason won't have SISL available but expect it to be there.

Just add a USE_SISL option that people can turn to OFF explicitely if they don't want SISL.

@2maz 2maz force-pushed the fix_sisl_optional branch 2 times, most recently from fcbef47 to dc1aaea Compare April 4, 2019 21:35
@2maz
Copy link
Member Author

2maz commented Apr 4, 2019

Updated

@2maz 2maz force-pushed the fix_sisl_optional branch from dc1aaea to 43604ec Compare April 12, 2019 08:13
@planthaber
Copy link
Member

Please don't make it dependent on the presence or not of SISL, which will only cause pain for people that for whatever reason won't have SISL available but expect it to be there.

Isn't this the default way ./configure scripts and any other installation of software with optional dependencies work?

Even in most CMake libs, when a lib is not found I get a warning and have to provide e.g. SISL_PATH manually when it is not found.

What about a WITH_SISL define in Trajectory.hpp header that removed the code and produces an error "base-types was compiled without SISL, base::Trajectory is not available" on compile time when the header is used?

@2maz
Copy link
Member Author

2maz commented Apr 17, 2019

What about a WITH_SISL define in Trajectory.hpp header that removed the code and produces an error "base-types was compiled without SISL, base::Trajectory is not available" on compile time when the header is used?

Regarding the discussion on base/orogen/types then SISL is rather a dependency for Rock, while it should still be optional in base/types to facilitate the external use.
Thus I agree with @doudou to have the option and thereby an explicit way of controlling the need for SISL, while having a error reported at configuration of base/types if SISL is missing.

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