-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PR196 1/3+] Generate .catkin develspace manifest file #271
Conversation
43913c2
to
7316080
Compare
… file before building any packages stages: Fixing stage validation to allow unicode commands
7316080
to
257120d
Compare
os.path.join(context.source_space_abs, path) | ||
for path, pkg in all_packages | ||
if path in dot_catkin_paths or path in packages_to_be_built_paths | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I care for this approach. The problem I have with it (unless I'm not interpreting the change correctly) is that packages which have not been built yet are already in the .catkin
file when the first package is built. This is different than what happens normally (without environment caching) and I'm not sure what the consequences might be. At the very least I think it could lead to subtle bugs which only show up (or are only suppressed) when using catkin_tools, but do not when using something like catkin_make_isolated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this will only add packages which are requested to be built.
It's different than catkin_make_isolated
(where each package gets added once it's configured and built), but more similar to catkin_make
(where each package gets added during the configure stage).
I agree that this behavior is slightly different, but it's only slightly different on the first build. Successive builds with CM and CMI work with an already-populated .catkin
file. If anything this makes the conditions of initial builds more similar to successive builds.
What we do know is that letting Catkin CMake populate the file in parallel has lead to malformed .catkin
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the parallel access to the .catkin
file is bad, but I'm still not convinced that env hooks shouldn't be able to calculate changes to an environment variable based on the state of the system which might change after each package is installed to the destination.
The reason I say that is that in the case of the parallel access there is a better approach to achieve the same goal. But for the case of dynamic env hooks, if someone is using them, (for example to calculate something based on what packages are installed to the destination), then there is no good alternative without support from the building tool.
This is what brings caching the environment hook changes into question for me.
Since you added an option to avoid env caching, however, I think this is about ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you added an option to avoid env caching, however, I think this is about ready to merge.
Yeah, I agree it should be optional for now. I'll make sure to add some documentation on it and pros/cons over in #250. Anything else this needs before merge? I also like this implementation much more than the one that supports the linked devel space in #196.
[PR196 1/3+] Generate .catkin develspace manifest file
Taking over responsibility for populating and updating the
.catkin
devel manifest file. See: #249 (comment)This also makes catkin "parallel-safe" mode unnecessary, i.e. ros/catkin#675
@cmansley