ensure exec dependencies are sourced when testing #86
Conversation
|
Do I read the patch correctly that the |
|
No that is not correct. It is only added to the test file. |
|
Oh I see. It only happens in the |
|
As you mentioned the current patch uses all exec dependencies without considering if they are in the workspace. I think this should be changed and checked before hand and not rely on the shell script to to the file existence check. |
|
I'll see if I can change it to do that. |
wjwwood
force-pushed
the
multiple-vendors
branch
from
e0adefd
to
1d90d70
Compare
|
I think 1d90d70 will accomplish that (I tested it locally and it works). I'll run CI over all of these things once I have followed up on everything. |
| @@ -225,6 +225,7 @@ def iterate_packages(opts, packages, per_package_callback): | |||
| opts.skip_packages = opts.skip_packages or [] | |||
| install_space_base = opts.install_space | |||
| package_dict = dict([(path, package) for path, package, _ in packages]) | |||
| workspace_package_names = sorted([pkg.name for pkg in package_dict.values()]) | |||
There was a problem hiding this comment.
sorted isn't necessary since the result isn't used for a loop. For performance reasons I would suggest to remove it.
There was a problem hiding this comment.
Ok, I removed it: 24a53fe
|
LGTM. |
wjwwood
force-pushed
the
multiple-vendors
branch
from
1d90d70
to
24a53fe
Compare
wjwwood
force-pushed
the
multiple-vendors
branch
from
24a53fe
to
a0bada4
Compare
This came up when I removed the unnecessary
test_dependonament_index_pythonafter a discussion here: ros2/rclpy#5 (comment)I believe this is the correct course of action for now. Basically every step of the build process (build, test, install, uninstall) can produce a "prefix" file which is a shell script which is placed before the command being run (make, ctest, msbuild, whatever) that can source setup files from other packages. Right now, all of these files will try to source the package specific local setup file for each package which comes before it in topological order (basically that packages direct build/test/buildtool dependencies and the recursive run dependencies of those direct dependencies). Because of this, dependencies which are listed as an exec depend only (not also a build/buildtool/exec depend) would not be sourced before running tests because it would not make into the test prefix file (usually named
cmake__test.{sh|bat}).This pull request extends what was already being added by including the exec depends of the package being tested. Some of these dependencies may not be packages in the repository, but all source lines are guarded by an "if-exists-then-source" type of logic, so it's ok.
Connects to ros2/rclpy#5