Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 14, 2016
Merged

[PR196 1/3+] Generate .catkin develspace manifest file #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion catkin_tools/execution/stages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@

import os

try:
basestring
except NameError:
basestring = str

try:
from shlex import quote as cmd_quote
except ImportError:
Expand Down Expand Up @@ -76,7 +81,7 @@ def __init__(
:param logger_factory: The factory to use to construct a logger (default: IOBufferProtocol.factory)
"""

if not type(cmd) in [list, tuple] or not all([type(s) is str for s in cmd]):
if not type(cmd) in [list, tuple] or not all([isinstance(s, basestring) for s in cmd]):
raise ValueError('Command stage must be a list of strings: {}'.format(cmd))
super(CommandStage, self).__init__(label, logger_factory, occupy_job)

Expand Down
36 changes: 35 additions & 1 deletion catkin_tools/verbs/catkin_build/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,43 @@ def build_isolated_workspace(
else:
break

# Populate .catkin file if we're not installing
# NOTE: This is done to avoid the Catkin CMake code from doing it,
# which isn't parallel-safe. Catkin CMake only modifies this file if
# it's package source path isn't found.
if not context.install:
wide_log("[build] Checking devel manifest...")
dot_catkin_file_path = os.path.join(context.devel_space_abs, '.catkin')
# If the file exists, get the current paths
if os.path.exists(dot_catkin_file_path):
dot_catkin_paths = open(dot_catkin_file_path, 'r').read().split(';')
else:
dot_catkin_paths = []

# Update the list with the new packages (in topological order)
packages_to_be_built_paths = [
path
for path, pkg in packages_to_be_built
]

new_dot_catkin_paths = [
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
]
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.


# Write the new file if it's different, otherwise, leave it alone
if dot_catkin_paths == new_dot_catkin_paths:
wide_log("[build] Devel manifest is up to date.")
else:
wide_log("[build] Updating devel manifest.")
open(dot_catkin_file_path, 'w').write(';'.join(new_dot_catkin_paths))

# Get the names of all packages to be built
packages_to_be_built_names = [p.name for _, p in packages_to_be_built]

# Construct jobs
jobs = []
packages_to_be_built_names = [p.name for _, p in packages_to_be_built]
for pkg_path, pkg in all_packages:
if pkg.name not in packages_to_be_built_names:
continue
Expand Down