Fix export of optional parts in base/types #29

2maz · 2019-04-04T16:58:46Z

@doudou Any suggestions to make the Opaques part truely optional. Right now I assume I have to adapt touch orogen to permit injection of extra build flags in base-typekit.pc.in?

2maz · 2019-04-04T22:17:33Z

Depends on orocos-toolchain/orogen/pull/117

doudou · 2019-04-05T17:31:00Z

I guess one way would be to define something in the opaque wrapper, and use this in the Opaques.cpp code. In any case, I don't see a way to not have to define something.

2maz · 2019-04-08T14:57:29Z

@doudou any comments then on the current approach, which requires also a minimal change in orogen?

doudou · 2019-04-08T19:38:06Z

base.orogen

-if has_library?('base-lib')
+sisl_available = has_library?('base-lib-sisl')
+
+if sisl_available


This is inconsistent with the next line. It's either both base-lib or both base-lib-sisl.

changed it to base-types-sisl

doudou · 2019-04-08T19:41:30Z

CMakeLists.txt

    list(APPEND TOOLKIT_ADDITIONAL_SOURCES ${CMAKE_SOURCE_DIR}/src/spline.cpp)
-    list(APPEND TOOLKIT_ADDITIONAL_LIBRARIES ${BASELIB_LIBRARIES} ${SISL_LIBRARIES})
+    list(APPEND TOOLKIT_ADDITIONAL_LIBRARIES ${SISL_LIBRARIES})
+    list(APPEND PKG_CFLAGS "-DWITH_SISL=1")


Rather than this being done implicitly. I would actually prefer adding an explicit add_definition to OroGen's typekit definition, and have it code-generate the add_definitions. Same for the additional sources. It would place all of the logic in the orogen file, which feels better somehow.

The addition of SISL in TOOLKIT_ADDITIONAL_LIBRARIES feels weird here by the way ... Since we're using_library it should not be needed - which would simplify the whole block to only be adding src/spline.cpp.

Rather than this being done implicitly. I would actually prefer adding an explicit add_definition to OroGen's typekit definition, and have it code-generate the add_definitions. Same for the additional sources. It would place all of the logic in the orogen file, which feels better somehow.

You mean the following way:

using_library 'base-lib-sisl' typekit.add_definition("WITH_SISL",1)

Something like that, yes.

doudou · 2019-04-08T19:41:42Z

CMakeLists.txt


+pkg_check_modules(BASELIB REQUIRED "base-lib")
+include_directories(${BASELIB_INCLUDE_DIRS})
+list(APPEND TOOLKIT_ADDITIONAL_LIBRARIES ${BASELIB_LIBRARIES})


This toolkit_additional_libraries is puzzling me ...

This has been part of the previously sisl-dependent block. Since it is not optional anymore I updated it. So 'puzzling' means 'is not needed'?

I got that it was there ... but I wonder why (if ?) it is needed.

All types are contained in base-types library. So I removed the base-lib part.

planthaber · 2019-04-16T13:13:26Z

Is this flag really needed in the rock context?

I mean SISL is an additional, mostly not needed dependency when you want to use base-types with a library outside rock e.g. https://github.com/envire/envire-envire_core

This is why it should be truely optional in the base-types repo, but i don't see why it should be in the orogen component, the manifest.xml of base-types lists it as optional, but it is defined in the package_set, so it is installes automatically and availabel anyways.

2maz · 2019-04-17T09:00:34Z

Is this flag really needed in the rock context?

I do not have a real use case, but was only fixing the existing setup.

but it is defined in the package_set, so it is installes automatically and availabel anyways.

You can still control the dependencies that you install via exclude_packages in the manifest. So that is not a good reason.

This is why it should be truely optional in the base-types repo, but i don't see why it should be in the orogen component, the manifest.xml of base-types lists it as optional, but it is defined in the package_set, so it is installes automatically and availabel anyways.

Depends whether other reasons are brought up to support this optionality.
I prefer to reduce complexity and we could make base/orogen/types depend directly on base-types-sisl.

doudou · 2019-04-17T14:47:12Z

I prefer to reduce complexity and we could make base/orogen/types depend directly on base-types-sisl.

Since the consensus it to remove the optionality on base/orogen/types, then let's. SISL is part of "Core Rock", base/orogen/types should depend on it unconditionally.

If all in agreement, please close this PR.

2maz · 2019-04-17T15:16:23Z

Updated the PR to do the 'cleanup' of the optional parts

doudou · 2019-04-24T13:47:03Z

Would it make sense to add an explicit dependency on external/sisl ? With a comment to explain why ?

planthaber · 2019-04-24T13:59:19Z

Good question.

It would prevent a failing build without explanation in case someone writes a new package_set including base/types and base/orogen/types, but how likely is this?

2maz · 2019-04-24T14:22:36Z

Would it make sense to add an explicit dependency on external/sisl ? With a comment to explain why ?

Explanation definitely makes sense.
So how about going back to

using_library 'base-types'

add 'external/sisl' to the manifest.xml,
and add the check for base-types-sisl in the CMakeLists.txt to output the proper failure, explanation and error handling to the user.

doudou · 2019-04-24T16:15:14Z

Would it make sense to add an explicit dependency on external/sisl ? With a comment to explain why ?

Explanation definitely makes sense.
So how about going back to
using_library 'base-types'
add 'external/sisl' to the manifest.xml,
and add the check for base-types-sisl in the CMakeLists.txt to output the proper failure, explanation and error handling to the user.

Sounds good

2maz · 2019-04-25T11:44:51Z

Updated

2maz added WIP Do not merge labels Apr 4, 2019

2maz mentioned this pull request Apr 4, 2019

Make SISL truely optional rock-core/base-types#134

Merged

2maz force-pushed the fix_optional_exports branch from 76dd20d to e6bb191 Compare April 4, 2019 22:08

doudou reviewed Apr 8, 2019

View reviewed changes

2maz force-pushed the fix_optional_exports branch from e6bb191 to 89dedf3 Compare April 12, 2019 08:43

2maz removed Do not merge WIP labels Apr 12, 2019

2maz force-pushed the fix_optional_exports branch from 89dedf3 to 2c527ab Compare April 17, 2019 15:13

2maz force-pushed the fix_optional_exports branch from 2c527ab to 01cbf60 Compare April 25, 2019 11:41

Make SISL a dependency of base/orogen/types

d7f7b39

2maz force-pushed the fix_optional_exports branch from 01cbf60 to d7f7b39 Compare April 25, 2019 11:44

doudou merged commit ccd43fe into master Apr 25, 2019

doudou deleted the fix_optional_exports branch April 25, 2019 22:04

Fix export of optional parts in base/types #29

Fix export of optional parts in base/types #29

Uh oh!

Conversation

2maz commented Apr 4, 2019

Uh oh!

2maz commented Apr 4, 2019

Uh oh!

doudou commented Apr 5, 2019

Uh oh!

2maz commented Apr 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

planthaber commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2maz commented Apr 17, 2019

Uh oh!

doudou commented Apr 17, 2019

Uh oh!

2maz commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doudou commented Apr 24, 2019

Uh oh!

planthaber commented Apr 24, 2019

Uh oh!

2maz commented Apr 24, 2019

Uh oh!

doudou commented Apr 24, 2019

Uh oh!

2maz commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

planthaber commented Apr 16, 2019 •

edited

Loading

2maz commented Apr 17, 2019 •

edited

Loading