[PR196 1/3] New asyncio-based execution engine #249

jbohren · 2015-12-15T09:10:52Z

This PR is the first set of refactored partial changes from PR #196 relating only to the execution engine and I/O handling, without the "linked" develspace support nor improved catkin clean support. It also includes the changes from #247 (and supersedes #248). This is meant to be a "functional" PR and it is expected that it might need further small revisions before being merged. This also adds a Travis CI configuration that builds on Linux as well as OS X.

Main Module Changes

`catkin_tools/common.py`

The changes in catkin_tools.common include the following:

Replacing FakeLock with an asyncio-based implementation
Addition of get_recursive_build_dependants_in_workspace function which gives the packages that depend on a given package in the workspace. (This is used in the future)
Replacement of os.Popen with subprocess.Popen
Fixing bug where slice_to_printed_length fails if the length is larger than the length of the lookup array.
Fixing uncaught IOError when the program receives a sigint during a wide_log message
Adds a mkdir_p function which recursively makes directories

`catkin_tools/resultspace.py`

The main change to catkin_tools.resultspace is the addition of environment and env hook checksum caching. This enables faster builds by using the resultspace mechanism instead of the build_env.sh mechanism. The resultspace loading can also cache the environment, and updates the cache when a resultspace's Catkin env_hooks change in any way.

It also adds a strict flag which controls whether or not to check for a .catkin file before loading the environment.

`catkin_tools/argument_parsing.py`

The changes in catkin_tools.argument_parsing mainly support the new job server, which is initialized even when there isn't any support for the GNU Make jobserver, to allow for future extensions to the jobserver in other contexts (distcc, ninja, etc).

Execution Engine

The execution engine consists of the main asyncio-based Executor, Jobs, and job Stages described in #196. Jobs for different catkin build types are defined via setuptools entry_points. This improves job parallelism, but also provides a mechanism for capturing stdout and stderr independently. As such, warnings and errors are now detected and printed in a much clearer manner.

This PR improves on the CMake and Catkin jobs defined in #196 by forgoing using the build_env.sh script, and instead using the catkin_tools.resultspace-based environment loading. This not only speeds up builds, but it also fixes a bug that prevented single-pass workspace building as described in this thread.

More details on the design of the execution engine are forthcoming via a documentation PR #250

Job Environments

Previously, each build job got its environment from a build_env.sh file, which was generated before the package was built. This shell script essentially sources the resultspace that each job is meant to be built against.

This PR adds the ability to cache the resulting environment from sourcing these resultspace setup file. In addition to speeding up all builds, it speeds up building already-built packages dramatically. For the Hydro ros-base workspace (172 packages), catkin build without env caching takes about 30 seconds, and catkin build with env caching takes about 10 seconds. This means that it's saving between 0.1-0.2 seconds per package, which adds up. Note that this effect becomes more dramatic as more workspaces are chained.

This PR removes the build_env.sh files entirely, and instead opts for a non-static way to get the environment for a job. This is the --env option which can be passed to any verb which supports it. So to get the environment in which qt_gui_cpp is built, you could run catkin build --env qt_gui_cpp, and it will print the environment to stdout. This is used instead of build_env.sh to reproduce build stages as well, as shown below:

_________________________________________________________________________________
Warnings   << qt_gui_cpp:make /home/jbohren/ws/ct_roscomm/build/_logs/qt_gui_cpp/build.make.000.log
cd /home/jbohren/ws/ct_roscomm/build/qt_gui_cpp; catkin build --env qt_gui_cpp | xargs -I %ENV% env %ENV% /usr/bin/make --jobserver-fds=6,7 -j; cd -
sip: Deprecation warning: qt_gui_cpp.sip:1: %Module version number should be specified using the 'version' argument
** WARNING expected ``)'' = 40
.................................................................................

Since this feature changes behavior, it is off by default, but can be enabled / disabled with --env-cache and --no-env-cache, respectively.

Addresses the Following Issues

Outstanding Issues

jbohren · 2015-12-15T09:17:20Z

catkin_tools/verbs/catkin_build/build.py

+        log(clr("[build] @!@{rf}Error:@| The workspace packages have a circular "
+                "dependency, and cannot be built. Please run `catkin list "
+                "--deps` to determine the problematic package(s)."))
+        return


The above provides a more graceful failure for #229

xqms · 2015-12-15T10:33:26Z

Sorry, answered to the wrong PR. I'm putting this here again so it's in the right place ;-)

Thanks for your work on separating this!

I just saw a unicode exception here:

Warnings << gazebo_ros_control:make /var/lib/jenkins/jobs/spacebot/workspace/build/_logs/gazebo_ros_control/build.make.000.log
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/home/max/install/catkin_tools/catkin_tools/execution/controllers.py", line 452, in run
    wide_log('\n'.join(lines))
  File "/home/max/install/catkin_tools/catkin_tools/common.py", line 450, in wide_log
    wide_log_fn(msg, **kwargs)
  File "/home/max/install/catkin_tools/catkin_tools/common.py", line 419, in disabled_wide_log
    log(msg, **kwargs)
  File "/home/max/install/catkin_tools/catkin_tools/common.py", line 272, in log
    print(*args, **kwargs)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2018' in position 129: ordinal not in range(128)

The unicode character 2018 is "LEFT SINGLE QUOTATION MARK", emitted by gcc on my system:

[...] warning: enumeration value ‘EFFORT’ not handled in switch

xqms · 2015-12-15T10:37:30Z

Sorry, this may have been caused by running catkin_tools under Jenkins, which apparently sets stdout encoding to ascii. Running catkin build straight in the terminal works just fine.

I'll investigate further.

jbohren · 2015-12-15T14:52:56Z

Sorry, this may have been caused by running catkin_tools under Jenkins, which apparently sets stdout encoding to ascii. Running catkin build straight in the terminal works just fine.

@xqms Yeah, overall, the unicode encoding/decoding needs to be cleaned up before or after this gets merged. Can you simulate the stdout encoding in a unit test?

xqms · 2015-12-16T18:11:15Z

For reference, I was able to fix my issue by setting the environment variable PYTHONIOENCODING=utf_8 when executing catkin_tools under jenkins. This forces the stdout encoding to UTF-8.

@xqms Yeah, overall, the unicode encoding/decoding needs to be cleaned up before or after this gets merged. Can you simulate the stdout encoding in a unit test?

I guess we should define the wanted behavior first. I'd propose that bytes captured from a job should be passed through 1:1 without caring about the encoding. First decoding to unicode and then re-encoding again to some variable output encoding is just asking for trouble.

This might conflict with the need to actually parse the output (e.g. line splitting). Checking for the newline byte should still be possible without decoding to unicode, though...

NikolausDemmel · 2015-12-16T23:51:55Z

One question: Why are we downloading and compiling catkin from github and then sourcing the resulting setup.bash in the travis config? It turns out that (on my OS X machine) 5 tests fail without this, but why should that be a prerequisite?

jbohren · 2015-12-17T00:20:47Z

One question: Why are we downloading and compiling catkin from github and then sourcing the resulting setup.bash in the travis config?

Well, we're getting catkin from github on Travis because it's a prerequisite for building the Catkin packages.

It turns out that (on my OS X machine) 5 tests fail without this, but why should that be a prerequisite?

@NikolausDemmel Which tests are failing, and are you running the tests in a clean environment, or after sourcing some setup file? If it's a clean environment, I would expect any tests which build Catkin CMake packages to fail.

NikolausDemmel · 2015-12-17T00:33:37Z

Well, we're getting catkin from github on Travis because it's a prerequisite for building the Catkin packages.

Would it make sense to have a copy of catkin in the test suite for that?

Which tests are failing, and are you running the tests in a clean environment, or after sourcing some setup file? If it's a clean environment, I would expect any tests which build Catkin CMake packages to fail.

Clean environment w-r-t ROS, i.e. I haven't sourced any setup file, catkin is not available. It makes sense as you say. Output of test command is: https://gist.github.com/NikolausDemmel/b7e5acea3ec3539672c3#file-catkin_tools-nosetests

jbohren · 2015-12-17T01:06:33Z

Well, we're getting catkin from github on Travis because it's a prerequisite for building the Catkin packages.

Would it make sense to have a copy of catkin in the test suite for that?

I don't think that's necessary. It might be best to fix the version (or even add more versions to the matrix), though. What do you think, @wjwwood?

Which tests are failing, and are you running the tests in a clean environment, or after sourcing some setup file? If it's a clean environment, I would expect any tests which build Catkin CMake packages to fail.

Clean environment w-r-t ROS, i.e. I haven't sourced any setup file, catkin is not available. It makes sense as you say. Output of test command is: https://gist.github.com/NikolausDemmel/b7e5acea3ec3539672c3#file-catkin_tools-nosetests

Yeah, that's what I would expect, all's good.

NikolausDemmel · 2015-12-17T12:57:44Z

I don't think that's necessary. It might be best to fix the version (or even add more versions to the matrix), though.

I just find it a bit strange that after downloading catkin_tools and installing all python and system dependencies you can't successfully run the unit tests without making sure catkin is in your environment. That's why I think keeping a copy to make the tests self-contained might be reasonable. It would also fix the version of catkin that is tested against and can be updated explicitely when needed.

But in any case there should be some documentation about this if catkin is not included. I believe on your docs branch you have already some stuff for developers, like how to add verbs etc, so that might be the best place.

jbohren · 2015-12-17T13:24:22Z

I don't think that's necessary. It might be best to fix the version (or even add more versions to the matrix), though.

I just find it a bit strange that after downloading catkin_tools and installing all python and system dependencies you can't successfully run the unit tests without making sure catkin is in your environment. That's why I think keeping a copy to make the tests self-contained might be reasonable. It would also fix the version of catkin that is tested against and can be updated explicitely when needed.

Yeah, I'm just always weary of copying source code around. If anything, I'd have the tests check it out from source and build/source it.

But in any case there should be some documentation about this if catkin is not included. I believe on your docs branch you have already some stuff for developers, like how to add verbs etc, so that might be the best place.

That sounds reasonable, I'll add that to #250.

wjwwood · 2015-12-17T18:20:23Z

My preference would be to have the tests check out a copy of catkin during the test setup. Other necessities like catkin_pkg and empy are actually dependencies (directly or indirectly) so they should be installed when you run the tests.

mikepurvis · 2015-12-17T18:22:10Z

By necessity, catkin_tools and catkin are joined at the hip— it makes sense to have tests on catkin_tools which will detect breaking changes in catkin proper.

wjwwood · 2015-12-17T18:25:48Z

By necessity, catkin_tools and catkin are joined at the hip— it makes sense to have tests on catkin_tools which will detect breaking changes in catkin proper.

I don't actually agree with the first half of that statement 😄. It should be perfectly capable (in the future if not now) of installing a workspace full of pure cmake packages.

But I do agree that the main use case is to use it in conjunction with catkin. It's not clear to me what version of catkin it should checkout, e.g. the indigo-devel branch or the latest release tag and in the future when there are multiple "latest" releases to choose from, which one? It might even require it to be a configuration of the tests and to be added to the matrix of things to test.

jbohren · 2015-12-18T00:21:09Z

But I do agree that the main use case is to use it in conjunction with catkin. It's not clear to me what version of catkin it should checkout, e.g. the indigo-devel branch or the latest release tag and in the future when there are multiple "latest" releases to choose from, which one? It might even require it to be a configuration of the tests and to be added to the matrix of things to test.

Here's the simplest way to implement this, as shown over in #251
https://github.com/catkin/catkin_tools/blob/pre-0.4.0-destdir/.travis.yml#L14

wjwwood · 2015-12-21T04:13:13Z

.travis.before_install.bash

+    #sudo add-apt-repository ppa:fkrull/deadsnakes -y
+    #sudo apt-get update
+    #sudo apt-get install python3.4 python3-dev
+  #fi


This looks like it should be removed?

Yeah this is left over from before 3.4 was available on Travis Precise.

wjwwood · 2015-12-21T04:42:52Z

Does this need to be rebased?

jbohren · 2015-12-21T12:09:00Z

Does this need to be rebased?

@wjwwood I don't think it needs to be rebased, it built on #247 and doesn't interfere with the other ones that you merged yesterday.

wjwwood · 2016-01-19T23:48:39Z

You need to lead the output with '\r' + (' ' * terminal_width()) + <output>, otherwise you get lines like this: [100%] Built target cpp_common] [1/4 jobs] [64 queued] [High Load] [cpp_common:make (100%) - 2.7]

wjwwood · 2016-01-19T23:49:11Z

Sorry that should be: '\r' + (' ' * terminal_width()) + '\r' + <output>

wjwwood · 2016-01-19T23:56:41Z

Also, should the warnings be re-quoted when using -i? I get stuff like this:

[ ... ]
-- Found PY_em: /Library/Python/2.7/site-packages/em.pyc
-- Using empy: /Library/Python/2.7/site-packages/em.pyc
-- Using CATKIN_ENABLE_TESTING: ON
-- Call enable_testing()
-- Using CATKIN_TEST_RESULTS_DIR: /Users/william/indigo/build/rosconsole_bridge/test_results
-- Found gtest: gtests will be built
-- Using Python nosetests: /usr/local/bin/nosetests-2.7
-- catkin 0.6.16
-- Configuring done
CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
-- Generating done
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   rosconsole_bridge

This warning is for project developers.  Use -Wno-dev to suppress it.

-- Build files have been written to: /Users/william/indigo/build/rosconsole_bridge
_________________________________________________________________________________________________________________________________________________________________________________
Warnings   << rosconsole_bridge:cmake /Users/william/indigo/build/_logs/rosconsole_bridge/build.cmake.000.log
CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   rosconsole_bridge

This warning is for project developers.  Use -Wno-dev to suppress it.

cd /Users/william/indigo/build/rosconsole_bridge; catkin build --get-env rosconsole_bridge | catkin env -si  /usr/local/bin/cmake /Users/william/indigo/src/rosconsole_bridge --no-warn-unused-cli -DCATKIN_DEVEL_PREFIX=/Users/william/indigo/devel -DCMAKE_INSTALL_PREFIX=/Users/william/indigo/install; cd -
.................................................................................................................................................................................

I haven't checked the behavior when using -v.

wjwwood · 2016-01-19T23:59:15Z

Another small thing I noticed are snippets like this one: @{yf}{cf}--@{yf}| Found the following Boost libraries: which appear to have been skipped over when doing substitutions.

wjwwood · 2016-01-20T00:00:26Z

Other than the overwriting of the status line when using -i, feel free to ticket the additional items to be done separately from this pr.

… warnings on interleaved, and fixing CMake output parsing bug

jbohren · 2016-01-20T00:35:34Z

@wjwwood OK. All the console controller issues are resolved in 0359197 / ff9a92a

wjwwood · 2016-01-20T02:09:03Z

I think we need to print the status line after new input, but there might a performance trade-off.

For now I think it's good enough; we can address things after merging.

wjwwood · 2016-01-20T02:10:40Z

I also think we may not be closing out child processes correctly. But I can investigate that more later.

[PR196 1/3] New asyncio-based execution engine

jbohren · 2016-01-20T02:14:43Z

I think we need to print the status line after new input, but there might a performance trade-off.

Yeah, I was thinking that, too.

For now I think it's good enough; we can address things after merging.

👏 Awesome.

NikolausDemmel · 2016-01-20T06:32:10Z

Nice!!

xqms · 2016-01-20T10:59:23Z

Congrats on getting this merged - I think this has been a major step forward :-)

jbohren added the enhancement label Dec 15, 2015

jbohren added this to the 0.4.0 - Second Beta Announcement milestone Dec 15, 2015

This was referenced Dec 15, 2015

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

Closed

New asyncio-based execution engine (PR#196 1/3) #248

Closed

jbohren reviewed Dec 15, 2015
View reviewed changes

jbohren force-pushed the pre-0.4.0-executor branch from aadf139 to 78f8d4b Compare December 16, 2015 01:11

jbohren mentioned this pull request Dec 16, 2015

destdir: Resurrecting destdir support, adding test case (PR#196) #251

Merged

jbohren mentioned this pull request Dec 17, 2015

[PR196 *] Adding documentation for forthcoming changes in 0.4.0 #250

Closed

18 tasks

jbohren mentioned this pull request Dec 18, 2015

[PR196-mod] Cleaning up use of build env and adding repro messages #254

Merged

jbohren changed the title ~~New asyncio-based execution engine (PR#196 1/3)~~ [PR196 1/3] New asyncio-based execution engine Dec 18, 2015

NikolausDemmel mentioned this pull request Dec 21, 2015

Add roslint verb #241

Closed

wjwwood reviewed Dec 21, 2015
View reviewed changes

jbohren added 2 commits January 19, 2016 19:29

controllers: Fixing wide-log interleaved output, suppressing buffered…

0359197

… warnings on interleaved, and fixing CMake output parsing bug

io: Fixing buffered io splitter for strict typing

ff9a92a

wjwwood added a commit that referenced this pull request Jan 20, 2016

Merge pull request #249 from catkin/pre-0.4.0-executor

c5c8807

[PR196 1/3] New asyncio-based execution engine

wjwwood merged commit c5c8807 into master Jan 20, 2016

wjwwood deleted the pre-0.4.0-executor branch January 20, 2016 02:10

This was referenced Jan 22, 2016

[PR196 2/3] Adding linked develspace support #276

Merged

build: There should be a warning when there are missing links in the workspace dependency graph #257

Closed

build: install and develspace interplay #69

Closed

jbohren mentioned this pull request Mar 9, 2016

build: option to show compiler warnings #301

Closed

jbohren mentioned this pull request Apr 2, 2016

Display warnings without the -v verbose option #308

Closed

wjwwood mentioned this pull request Mar 24, 2017

Non-path envvars with colons get mangled #448

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PR196 1/3] New asyncio-based execution engine #249

[PR196 1/3] New asyncio-based execution engine #249

jbohren commented Dec 15, 2015

jbohren Dec 15, 2015

xqms commented Dec 15, 2015

xqms commented Dec 15, 2015

jbohren commented Dec 15, 2015

xqms commented Dec 16, 2015

NikolausDemmel commented Dec 16, 2015

jbohren commented Dec 17, 2015

NikolausDemmel commented Dec 17, 2015

jbohren commented Dec 17, 2015

NikolausDemmel commented Dec 17, 2015

jbohren commented Dec 17, 2015

wjwwood commented Dec 17, 2015

mikepurvis commented Dec 17, 2015

wjwwood commented Dec 17, 2015

jbohren commented Dec 18, 2015

wjwwood Dec 21, 2015

jbohren Dec 21, 2015

wjwwood commented Dec 21, 2015

jbohren commented Dec 21, 2015

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 20, 2016

jbohren commented Jan 20, 2016

wjwwood commented Jan 20, 2016

wjwwood commented Jan 20, 2016

jbohren commented Jan 20, 2016

NikolausDemmel commented Jan 20, 2016

xqms commented Jan 20, 2016

[PR196 1/3] New asyncio-based execution engine #249

[PR196 1/3] New asyncio-based execution engine #249

Conversation

jbohren commented Dec 15, 2015

Main Module Changes

catkin_tools/common.py

catkin_tools/resultspace.py

catkin_tools/argument_parsing.py

Execution Engine

Job Environments

Addresses the Following Issues

Enhancements

Definite Bugfixes

Likely Bugfixes (through different implementation)

Outstanding Issues

jbohren Dec 15, 2015

Choose a reason for hiding this comment

xqms commented Dec 15, 2015

xqms commented Dec 15, 2015

jbohren commented Dec 15, 2015

xqms commented Dec 16, 2015

NikolausDemmel commented Dec 16, 2015

jbohren commented Dec 17, 2015

NikolausDemmel commented Dec 17, 2015

jbohren commented Dec 17, 2015

NikolausDemmel commented Dec 17, 2015

jbohren commented Dec 17, 2015

wjwwood commented Dec 17, 2015

mikepurvis commented Dec 17, 2015

wjwwood commented Dec 17, 2015

jbohren commented Dec 18, 2015

wjwwood Dec 21, 2015

Choose a reason for hiding this comment

jbohren Dec 21, 2015

Choose a reason for hiding this comment

wjwwood commented Dec 21, 2015

jbohren commented Dec 21, 2015

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 19, 2016

wjwwood commented Jan 20, 2016

jbohren commented Jan 20, 2016

wjwwood commented Jan 20, 2016

wjwwood commented Jan 20, 2016

jbohren commented Jan 20, 2016

NikolausDemmel commented Jan 20, 2016

xqms commented Jan 20, 2016

`catkin_tools/common.py`

`catkin_tools/resultspace.py`

`catkin_tools/argument_parsing.py`