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.

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

[major] executor/clean: Adding per-package cleaning, linked develspaces, and a new execution pipeline #196

Closed
wants to merge 22 commits into from

Conversation

jbohren
Copy link
Contributor

@jbohren jbohren commented Apr 13, 2015

_NOTE: This has been superseded by #247, #249, #276, and #293._

Overview

This PR involves a dramatic refactor of the core execution pipeline for building and cleaning packages in order to implement two high-priority features:

  • Per-package / partial-workspace cleaning (reversible build actions)
    • Requires build "linked" develspaces, which are isolated per-package develspaces whose contents are symbolically-linked into a merged develspace
    • Requires using the catkin build execution pipeline outside of the build verb
  • (build: detect and report on build warnings when not verbose #1) Automatic stderr-based warning and error detection
    • Requires separating stdout from stderr during execution of each job

Additionally, addresses the following outstanding issues:

It also provides the following:

  • More transparency into what the catkin_tools executor is doing via named job "stages"
  • Enables stages of jobs which are implemented in pure python and don't require subprocesses
  • Clearer designation of errors and warnings when they are printed to the console
  • In order to re-generate warnings a --pre-clean option has been added to the build verb to make clean in the build directory before running make
  • In order to build unbuilt packages, an --unbuilt option has been added to the build verb to build any packages in the workspace which have not been built
  • More introspection into the build status by displaying cmake package build percentages in status line

It also resulted in some unexpected benefits:

  • Build times can be sped up dramatically for no-op packages since the env.sh files aren't needed for merged or linked develspaces, for example:
    • A workspace with 188 pre-built packages completes in ~17s instead of ~55s (3.2x speedup)

It also revealed the following problems:

Per-Package Cleaning and the Linked Devel Space

This PR provides support for cleaning individual packages with the following syntax:

catkin clean [-a | -b | -d | -i] [-f] [--orphans] [--deps] [--deinit] [[PKG] PKG ...]

Where -b means buildspace, -d means develspace, -i means installspace, and -a means all three. Other examples include:

catkin clean Examples

  • catkin clean / catkin clean -a
    • Clean the entire build, devel, and install spaces (currently supported)
  • catkin clean -af / catkin clean --all --force
    • Clean the entire build, devel, and install spaces without asking for verififcation
  • catkin clean -b
    • Clean just the build space (currently supported)
  • catkin clean pkg_a pkg_b
    • Clean pkg_a and pkg_b from the build, devel, and install spaces
  • catkin clean --deps pkg_a
    • Clean pkg_a as well as any packages which depend on pkg_a.
  • catkin clean --orphans
    • Clean all packages which have been built but are no longer in the source space.
  • catkin clean --deinit
    • De-initialize workspace (delete .catkin_tools) directory

Approach

Doing this right is hard, but it's an important feature. I think this sort of per-package clean functionality is one of the the final big features preventing catkin_tools from being complete. In complex workspaces with hundreds of packages, debugging can be a huge challenge, and this is only made more challenging by the inability to reliably remove all of the products of a specific packages from a workspace.

CMake Packages

Vanilla CMake packages are handled trivially since their install target is used to write files to devel and install spaces. This target creates an install_manifest.txt which lists all the generated files, so to clean CMake packages, we can simply remove these files.

The normal location for install_manifest.txt, however, is in the package's build space. This file could get overwritten if the user builds both a devel and install variant, so we copy this file into the appropriate result space for safe keeping. Also, the install_manifest.txt file does not specify directories which are created by cmake, so the CMake clean job will remove any directories which are made empty through the removal of files listed in the install_manifest.txt file.

Catkin Packages

Catkin packages prove more challenging for a few reasons. For Catkin packages, there are two different steps which generate files in the devel space: the CMake configure step, and the Make build step. It's possible to clean the files generated in the build step with the standard clean target, but this still leaves numerous files generated by the configure step. Catkin packages also each generate some of the same files such as the setup shell scripts and each write to .catkin file which lists all the source packages in the workspace. This all makes for a big mess to clean up.

The approach used in this PR is to build each package into an isolated devel space, and then symbolically link the contents into the merged devel space. This prototype behavior is enabled by passing the --link-devel option to catkin build. Since catkin_tools does the linking, it can generate a devel_manifest.txt similar to CMake's install_manifest.txt which logs all of the files that a given package contributes to the devel space.

If a file that was listed in the previous link step is no longer found in the isolated devel space, it is removed from the merged devel space.

The above handles tracking the files generated by each package, but doesn't handle collisions very well. In order to handle collisions, the symlink generation step counts the number of times a collision occurs for each file. This information is stored in a file called devel_collisions.txt.

In summary:

  • The isolated devel spaces are: DEVEL_PREFIX/.catkin_tools/PKG_NAME/linked_devel
  • The devel manifests are stored in: DEVEL_PREFIX/.catkin_tools/PKG_NAME/devel_manifest.txt
  • The collisions file is stored in: DEVEL_PREFIX/.catkin_tools/devel_collisions.txt
  • All files from each isolated devel space are linked into: DEVEL_PREFIX/

The clean step for a given package reads its devel_manifest.txt and for each file that was generated it either removes the file or decrements the count in the devel_collisions.txt file.

Setup File Generation

One challenge with this approach, as mentioned above, is that catkin packages normally generate several files in the root of the devel space used for "sourcing" that devel space. These include:

  • .catkin
  • .rosinstall
  • _setup_util.py
  • env.sh
  • setup.sh
  • setup.bash
  • setup.zsh

With the exclusion of .catkin, these files are all generated from catkin_generate_environment.cmake during the CMake configure step of a catkin package, and several of them include information computed by existing Catkin CMake code. Since some of these reference the absolute path of the develspace, they can't simply be copied from the isolated devel spaces of the constituent packages.

The simplest solution to this problem is to have a "ghost" bootstrap package called catkin_tools_bootstrap which gets generated the first time a given resultspace is built. This guarantees that the setup files are generated correctly by Catkin, itself.

A convenient side-effect of the catkin_tools_bootstrinstallap package is that it also enables catkin build to generate setup files for a develspace consisting only of vanilla CMake packages.

For the .catkin file, we make catkin_tools responsible for managing its contents. This is for two reasons:

  1. There have been observed problems building Catkin projects in parallel and writing to this file
  2. If we clean a package, we want to remove its source space from this file

Since catkin_tools will be modifying this file in parallel from a single process, we can manage exclusive access to it and prevent collisions. Additionally, we can remove packages from it when they are removed from the workspace.

Additional Notes

The current implementation works in Unix-based systems, but could be extended to Windows with one of these methods.

Architectural Changes

Executor

This PR completely re-implements the execution pipeline using osrf_pycommon and the trollius library (a Python2 and Python3 compatible version of Python3.4's asyncio. This yields an execution pipeline which can be used by catkin clean (and other potential verbs) and enables us to extract and display stdout and stderr separately. As such, it solves #1.

Execution Jobs and Stages

The new execution pipeline operates on a task model where a given "task" (build, clean, etc) can be decomposed into an acyclic "job" dependency graph, where each job is a sequence of "stages". Each job can be executed in parallel once its dependencies have been completed, and each stage within a job must be executed serially.

A Job is a simple container with a few non-mutating utility functions. It is designated with a unique Job ID (jid) and has a list of the jids of the jobs on which it depends. In catkin_build, these jids are normally package names.

A Stage is some atomic operation, which contains a non-unqie label (such as "cmake", "make", or "install"). There are currently two types of stages: CmdStage and FunStage. The CmdStage describes a system command. This type is similar to the current stage implementation, except it also facilitates using asynchronous protocols for capturing stdout and stderr. The FunStage, on the other hand, describes a blocking Python function. This second stage type enables us to affect the filesystem in arbitrary ways without spawning subprocesses or relying on CMake's shell utilities.

The construction of Job and Stage instances should have no side-effects, and are designed to be lazily evaluated only once their dependencies have been completed. Once a collection of Job objects have been created, they are passed directly to the executor.

Executing Jobs Asynchronously

The executor is an asyncio coroutine which executes each job asynchronously. Jobs follow a well-defined lifecycle. At any time, a given job is on one of the following lists:

  • pending - Jobs which are not ready to be executed (unfinished deps)
  • queue - Jobs which are ready to be executed
  • active - Jobs which are currently executing
  • abandoned - Jobs which were meant to be executed, but will not be attempted

By default, if one of the jobs fails, it will abandon all other active, queued, and pending jobs. This behavior can be controlled with the continue_on_failure and continue_without_deps options:

  • continue_on_failure - Don't abandon all jobs if one job fails.
  • continue_without_deps - Don't abandon the dependencies of failed jobs.

Jobs are activated subject to the availability of tokens from the job server. This happens whether or not the jobs erver is being used to manage make jobs since the job server treats the executor just like any other job client. Once a job is activated, each of its stages is executed in order, and if one stage fails, the job fails.

Getting Feedback from Execution

Like the current executor, the new execution pipeline uses a Queue to asynchronously communicate with the console thread. The executor writes ExecutionEvent objects into the event queue to be processed by any kind of output controller.

The default console controller has several options for output filtering and formatting:

  • show_stage_events False
  • show_buffered_stdout False
  • show_buffered_stderr True
  • show_live_stdout False
  • show_live_stderr False
  • show_active_status True
  • show_full_summary False
  • active_status_rate 20.0

Creating catkin build and catkin clean Jobs

Jobs for building and cleaning jobs are created by factory functions defined for catkin and cmake packages. This makes the job objects task-agnostic, even if they require different types of information, as seen in the case of catkin build and catkin clean.

Error Display

This PR dramatically simplifies the error output display in order to make errors clearer and less verbose. Since we have more controls to build single packages, it doesn't seem like it's necessary to have the "command to reproduce" and the numerous paths to different logfiles visible by default. On small screens, the actual error is usually even lost up above in the scrollback.

Current

current_warnings

This PR

cb_new_warnings

Cleaning Individual Packages

This PR adds jobs for cleaning all files generated by individual catkin and CMake packages in the build and devel spaces. The "clean" jobs are different from the "build" jobs since they do not technically need to know the source path to packages. In fact, it's desirable to make them only depend on the information in the devel_manifest.txt files to be cleaned. This way we can support full orphan removal if a package's source is removed from the workspace.

TODO

@wjwwood
Copy link
Member

wjwwood commented Apr 13, 2015

This moves Executor and the functions and classes which the Executor uses into a jobs module. This enables catkin clean (and other potential verbs) to take advantage of it.

I would have preferred doing this separately. It makes it really difficult to review things when you've mixed several large, somewhat unrelated, changes together. In the mean time you could have just imported what you needed from the Executor stuff while it remained in the build verb.

I think this sort of per-package clean functionality is the final big feature preventing catkin_tools from being complete.

I must have missed that road map 😛.

@jbohren
Copy link
Contributor Author

jbohren commented Apr 13, 2015

I would have preferred doing this separately. It makes it really difficult to review things when you've mixed several large, somewhat unrelated, changes together. In the mean time you could have just imported what you needed from the Executor stuff while it remained in the build verb.

The changes aren't hard to separate, and I can do that when this is ready for an in-depth review. The only reason why they're all together is that it was the quickest way to figure out what needed to move in order to make the clean functionality happen.

I think this sort of per-package clean functionality is the final big feature preventing catkin_tools from being complete.

I must have missed that road map 😛.

Meaning this is the last big feature that would enable me to spend as little time compiling code in my day-to-day work as I did with rosbuild 🤘

@wjwwood
Copy link
Member

wjwwood commented Apr 13, 2015

The changes aren't hard to separate, and I can do that when this is ready for an in-depth review. The only reason why they're all together is that it was the quickest way to figure out what needed to move in order to make the clean functionality happen.

That makes sense; I get that it is easier to develop like this. Like you said, we can split it up when it's time for review.

@jbohren
Copy link
Contributor Author

jbohren commented Apr 17, 2015

_NOTE: This information is out of date, please see the PR description for updated details._

@wjwwood So I added a couple functions to support symbolically linking isolated devel files into a merged devel space and creating a devel_manifest.txt similar to CMake's install_manifest.txt. I also crmigeated an experimental --link-devel argument to catkin configure , which right now simply builds into isolated develspaces contained in each package's build directory. I still need to incorporate the functions which actually create the symlinks.

What needs to be done now to support this in practice, is generating a merged devel space from a collection of isolated devel spaces. Besides copying all of the non-conflicting files, there are four files which are present in each devel space which would need to be properly merged or modified. Tell me if you think this is a correct assessment, and if you think it's reasonable for catkin_tools to be generating these files in the merged devel space or if catkin_tools should be leaning on Catkin for doing it.

@jbohren jbohren force-pushed the clean-pkgs branch 6 times, most recently from 4e8d0d0 to 74148f8 Compare April 18, 2015 16:30
@jbohren jbohren changed the title [experimental] clean: Big refactor of Executor and new per-package clean support [experimental] clean: Enable per-package clean support Apr 18, 2015
@jbohren jbohren changed the title [experimental] clean: Enable per-package clean support [experimental] clean: Enable per-package catkin clean support Apr 18, 2015
@jbohren
Copy link
Contributor Author

jbohren commented Apr 20, 2015

@wjwwood Check out the current design details in the updated PR description. I think it's actually converging to something that will work well in practice.

@davetcoleman
Copy link
Contributor

+1 i've always wanted this feature. i haven't looked at the actual code, though

@wjwwood
Copy link
Member

wjwwood commented Apr 22, 2015

Thanks for working on this @jbohren, it's on my list to review, and I'll get to it asap.

@jbohren jbohren changed the title [experimental] clean: Enable per-package catkin clean support [experimental] clean: Enable per-package catkin clean support via "linked" devel spaces Apr 22, 2015
@jbohren jbohren mentioned this pull request May 9, 2015
@scpeters
Copy link
Contributor

One caveat about using install_manifest.txt for vanilla cmake packages: this lists only installed files, not folders, which are automatically created as needed by the installer. Deleting the files in the install_manifest.txt won't be a complete clean if there are otherwise empty folders left behind. We noticed this behavior when implementing a make uninstall target in gazebo but haven't tried to fix it.

It's not a deal-breaker, and some people might not even notice, but it's worth keeping in mind.

@jbohren
Copy link
Contributor Author

jbohren commented May 23, 2015

One caveat about using install_manifest.txt for vanilla cmake packages: this lists only installed files, not folders, which are automatically created as needed by the installer. Deleting the files in the install_manifest.txt won't be a complete clean if there are otherwise empty folders left behind. We noticed this behavior when implementing a make uninstall target in gazebo but haven't tried to fix it.

This PR does in fact clean empty folders:
https://github.com/jbohren-forks/catkin_tools/blob/clean-pkgs/catkin_tools/jobs/cmake.py#L248-L274

@scpeters
Copy link
Contributor

Nice!

@jbohren jbohren force-pushed the clean-pkgs branch 3 times, most recently from 39a14ac to 03da6ec Compare May 24, 2015 16:55
@jbohren
Copy link
Contributor Author

jbohren commented May 24, 2015

@wjwwood I've integrated my experimental asyncio hacking into this branch. This still includes the per-package clean support and symlinked devel support. I still need to flesh out the rest of the logging, remove the old execution stuff, update the cli interface, and update both the PR design details and the catkin_tools documentation, but the tests are passing now.

@davetcoleman
Copy link
Contributor

@jbohren I am testing this pull request because I want to be able to remove individual packages without rebuilding my whole work space. After switching to your branch and running sudo python setup.py install catkin no longer works & I have an error. I am on ROS Jade and I get:

$ catkin build
Traceback (most recent call last):
  File "/usr/local/bin/catkin", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 2749, in <module>
    working_set = WorkingSet._build_master()
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 444, in _build_master
    ws.require(__requires__)
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 725, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/lib/python2.7/dist-packages/pkg_resources.py", line 628, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: trollius

Thoughts? Thanks!

@wjwwood
Copy link
Member

wjwwood commented Dec 5, 2015

@davetcoleman you must install Trollius (it is a Python2.7 compatible implementation of the Python3's asyncio). This should get you going:

$ sudo -H python2.7 -m pip install -U trollius

@davetcoleman
Copy link
Contributor

Thanks. Now I get:

ImportError: No module named osrf_pycommon.process_utils

It looks like osrf_pycommon is only available from source? I tried using a similar command to the one your just provided.

@wjwwood
Copy link
Member

wjwwood commented Dec 5, 2015

Yeah, you'll have to install that from source for now: https://osrf-pycommon.readthedocs.org/en/latest/#installing-from-source

@jeffeb3
Copy link

jeffeb3 commented Dec 5, 2015

Yes, its only from source AFAIK. Use pip from github:
http://stackoverflow.com/questions/8247605/configuring-so-that-pip-install-can-work-from-github
With the address pointing to the osrf_pycommon repo:
https://github.com/osrf/osrf_pycommon

On Sat, Dec 5, 2015, 11:18 AM Dave Coleman [email protected] wrote:

Thanks. Now I get:

ImportError: No module named osrf_pycommon.process_utils

It looks like osrf_pycommon is only available from soruce
https://osrf-pycommon.readthedocs.org/en/latest/? I tried using a
similar command to the one your just provided.


Reply to this email directly or view it on GitHub
#196 (comment).

@jeffeb3
Copy link

jeffeb3 commented Dec 5, 2015

Or that will work too.

On Sat, Dec 5, 2015, 11:24 AM Jeff Eberl [email protected] wrote:

Yes, its only from source AFAIK. Use pip from github:

http://stackoverflow.com/questions/8247605/configuring-so-that-pip-install-can-work-from-github
With the address pointing to the osrf_pycommon repo:
https://github.com/osrf/osrf_pycommon

On Sat, Dec 5, 2015, 11:18 AM Dave Coleman [email protected]
wrote:

Thanks. Now I get:

ImportError: No module named osrf_pycommon.process_utils

It looks like osrf_pycommon is only available from soruce
https://osrf-pycommon.readthedocs.org/en/latest/? I tried using a
similar command to the one your just provided.


Reply to this email directly or view it on GitHub
#196 (comment).

context=context,
env_file_path=env_file_path))

# Only use it for building if the develspace is isolated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbohren Can you explain the motivation here? This breaks building from scratch for me, e.g.

# Clean workspace (just src folder)
. /opt/ros/indigo/setup.bash
catkin build

This fails for packages including message headers built in other packages, since devel/include will not be included in the header search path. Indeed, how should a package know about the shared devel space if not through the env file?

After the first build fails, I can source the then-available devel/setup.bash and a second build will succeed.

If I use env_prefix = [env_file_path] unconditionally here, the devel/include is added to CPATH and everything works on the first build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xqms Yeah, this is a bug in the current PR. The original intention was to avoid re-sourcing the same setup files, but I left this in by mistake.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, you report a great speedup when avoiding re-sourcing the env.sh files above. Can we somehow keep that speedup? Maybe source one env.sh at the beginning of the build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can definitely avoid sourcing it with every command by only sourcing it once per package and saving the environment. We can also go a step further and bypass the environment files by sourcing the workspace setup file directl. The ultimate would be to only source it once per build and then re-source it only when the catkin env hooks change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xqms Take a look at #248 (which is #196 without linked devel or per-package cleaning). It does "smart" env loading and caching based on the env hooks.

@NikolausDemmel
Copy link
Member

For everyones reference: I have some minor tweaks to this PR here: jbohren-forks#17

@jbohren jbohren mentioned this pull request Dec 8, 2015
@NikolausDemmel
Copy link
Member

@jbohren, in the Context we now have isolate_devel and link_devel. It is unclear to me how they should affect each other and which one should take precedence. In the options we have --isolate-devel and --merge-devel, setting isolate_devel to True and False, respectively and --link-devel setting link_devel to True (which is also the default and therefore cannot currently be disabled). Would it make sense to rather have a single variable with 3 different possible values link, merge, isolate instead of these two booleans?

@jbohren
Copy link
Contributor Author

jbohren commented Dec 12, 2015

@NikolausDemmel Yeah, the interface should definitely be more enum-like. Currently there is no "precedence" they are all just mutually exclusive.

@NikolausDemmel
Copy link
Member

@jbohren @wjwwood any preferences on how enum style variables should be implemented? Using the Python 3.4 enum (there are pip-installable backports for earlier versions), or just something like a string?

@jbohren
Copy link
Contributor Author

jbohren commented Feb 12, 2016

I'm going to close this now that all of the contributions here have either already been merged or are represented in #250 and #293.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants