orocos-toolchain / orogen

Code generator for components and type handling in Rock - the Robot Construction Kit - and the Orocos Toolchain
http://rock-robotics.org
Other
4 stars 35 forks source link

fix default extensions being enabled twice in imported projects #115

Closed doudou closed 6 years ago

doudou commented 6 years ago

If one has two layers of imports (i.e. project A imports B imports C), then the last line of C's installed orogen file

Spec::TaskContext.enable_default_extensions

was unconditionally enabling the extensions, which leads to the default extensions being enabled in B (even though they should not).

Fix by using a push/pop model instead of a enable/disable model.

Fixes #114 Fixes https://github.com/rock-core/drivers-orogen-iodrivers_base/issues/18

doudou commented 6 years ago

Corresponding regression tests added with: https://github.com/rock-core/rock.build-tests-package_set/pull/1

doudou commented 6 years ago

I prefer avoiding 'unless' due to readability.

So did I, but then the default ruby style guide (as enforced by rubocop) recommends using it, so I started sticking to that.

doudou commented 6 years ago

I would recommend to add some comment why you actually use the enable/disable mechanism, i.e., why the explicit call to find_task_context(...).callToPlugin is needed for default extensions - it's still not clear to me at least.

Sure, I'll add it.

doudou commented 6 years ago

Comment: https://github.com/orocos-toolchain/orogen/blob/fix_default_extensions_being_enabled_twice/lib/orogen/templates/project.orogen#L10

Writing it lead to refactor the bugfix, to ensure that extensions that were default during generation are called at the same time (task context creation) instead of at the end of the orogen file. Corresponding build test: https://github.com/rock-core/build_tests-orogen-default_extensions/blob/master/default_extensions.orogen#L22

doudou commented 6 years ago

@2maz: does the thumbs up mean "tested, OK to merge" ?

2maz commented 6 years ago

Sorry, should have been more verbose. It means first of all thanks for fixing, and secondly good to merge - did pass testing with drivers/orogen/camera_usb.

doudou commented 6 years ago

Merged, thanks for the help !