Initial implementation #1
Conversation
|
CI will fail until |
cottsay
force-pushed
the
cottsay/implement
branch
from
563a8a5 to
9cc8697
Compare
| # Of those, ignore specifically allowed overrides | ||
| overriding_names.difference_update(args.allow_overriding) | ||
|
|
||
| # Of those, Ignore overrides which are (effectively) leaf packages |
There was a problem hiding this comment.
The way leaf packages are calculated looks fine to me. IIUC leaf packages eliminates the concern about ABI/API incompatibilities in the overridden package, but it doesn't eliminate the include directory search order problem. ros2/ros2#1150 is working to mitigate that for a lot of packages, but colcon doesn't have a way to know if a package has mitigated it. Since it can also lead to undefined behavior at runtime, I would recommend keeping the warning even when overridden packages are leaf packages in the underlay.
There was a problem hiding this comment.
I'm concerned that we'd be introducing a warning for what could be a valid workflow without giving the developer any way to resolve it properly, forcing them to suppress it. So even if the underlying packages have mitigated the problem, they'd still have to suppress the warning. That doesn't seem like a very good user experience, and will probably cause developers to simply ignore or disable these warnings because they "can't be fixed."
I think we need to come up with a plan for how to detect or declare "safe" packages before we do that, but I'm open to other thoughts.
There was a problem hiding this comment.
That makes sense. Its unfortunate that it means it won't warn in the example that motivated ros2/ros2#1150 https://github.com/jacobperron/overlay_bug_reproduction , but I suppose all the include directory stuff mitigates that.
There was a problem hiding this comment.
The fact that it's actually bit us in the wild is a strong case for maintaining some kind of warning for leafy sub-trees like that. I'm having trouble weighing the utility of warning in that case against a boy-who-cried-wolf sort of mentality being established regarding this warning.
Maybe we should disable this part and over-warn until we can come up with a way to detect or declare safe packages? Do we actually have any plans to do that?
There was a problem hiding this comment.
Maybe we should disable this part and over-warn until we can come up with a way to detect or declare safe packages? Do we actually have any plans to do that?
I don't think we have any plans. Given the volume of feedback against the first implementation of the warning, I think this PR strikes a good balance.
There was a problem hiding this comment.
LGTM with one nit. I tested locally that it warned in this case: https://github.com/sloretz/override_package_non_leaf/pull/1/files
Using these repos
vcs export output
repositories:
colcon-argcomplete:
type: git
url: https://github.com/colcon/colcon-argcomplete.git
version: master
colcon-bash:
type: git
url: https://github.com/colcon/colcon-bash.git
version: master
colcon-cd:
type: git
url: https://github.com/colcon/colcon-cd.git
version: master
colcon-cmake:
type: git
url: https://github.com/colcon/colcon-cmake.git
version: master
colcon-core:
type: git
url: https://github.com/colcon/colcon-core.git
version: cottsay/move-override-check
colcon-defaults:
type: git
url: https://github.com/colcon/colcon-defaults.git
version: master
colcon-devtools:
type: git
url: https://github.com/colcon/colcon-devtools.git
version: master
colcon-installed-package-information:
type: git
url: https://github.com/colcon/colcon-installed-package-information.git
version: cottsay/implement
colcon-library-path:
type: git
url: https://github.com/colcon/colcon-library-path.git
version: master
colcon-metadata:
type: git
url: https://github.com/colcon/colcon-metadata.git
version: master
colcon-mixin:
type: git
url: https://github.com/colcon/colcon-mixin.git
version: master
colcon-notification:
type: git
url: https://github.com/colcon/colcon-notification.git
version: master
colcon-output:
type: git
url: https://github.com/colcon/colcon-output.git
version: master
colcon-override-check:
type: git
url: https://github.com/colcon/colcon-override-check.git
version: cottsay/implement
colcon-package-information:
type: git
url: https://github.com/colcon/colcon-package-information.git
version: master
colcon-package-selection:
type: git
url: https://github.com/colcon/colcon-package-selection.git
version: master
colcon-parallel-executor:
type: git
url: https://github.com/colcon/colcon-parallel-executor.git
version: master
colcon-pkg-config:
type: git
url: https://github.com/colcon/colcon-pkg-config.git
version: master
colcon-powershell:
type: git
url: https://github.com/colcon/colcon-powershell.git
version: master
colcon-python-setup-py:
type: git
url: https://github.com/colcon/colcon-python-setup-py.git
version: master
colcon-recursive-crawl:
type: git
url: https://github.com/colcon/colcon-recursive-crawl.git
version: master
colcon-ros:
type: git
url: https://github.com/colcon/colcon-ros.git
version: cottsay/ament-index-deps
colcon-test-result:
type: git
url: https://github.com/colcon/colcon-test-result.git
version: master
colcon-zsh:
type: git
url: https://github.com/colcon/colcon-zsh.git
version: master
The logic implemented here is a fair amount more complex than the original approach. I made a substantial effort to eliminate the warnings where I'm moderately confident that overriding won't cause problems for typical cases. This needs some testing.
Some possible improvements:
Requires: colcon/colcon-core#475Requires: colcon/colcon-core#476
Requires: colcon/colcon-installed-package-information#1Requires: colcon/colcon-ros#138Closes: colcon/colcon-core#473
Closes: colcon/colcon-core#465
Might close: colcon/colcon-core#469