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

[PR196 1/3+] Making stage reproduction more portable, making resultspace sourcing more robust #273

Merged
merged 9 commits into from
Jan 19, 2016

Conversation

jbohren
Copy link
Contributor

@jbohren jbohren commented Jan 15, 2016

cd CWD; catkin build --get-env PKG | catkin env -si CMD; cd -

Full catkin env -h help:

usage: catkin env [-h] [-i] [-s]
                  [NAME=VALUE [NAME=VALUE ...]] [COMMAND] [ARG [ARG ...]]

Run an arbitrary command in a modified environment.

positional arguments:
  NAME=VALUE            Explicitly set environment variables for the
                        subcommand. These override variables given to stdin.

optional arguments:
  -h, --help            show this help message and exit
  -i, --ignore-environment
                        Start with an empty environment.
  -s, --stdin           Read environment variable definitions from stdin.
                        Variables should be given in NAME=VALUE format.

command:
  COMMAND               Command to run. If omitted, the environment is printed
                        to stdout.
  ARG                   Arguments to the command.

@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch from 930a402 to af54a6e Compare January 15, 2016 15:23
@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch 2 times, most recently from 2840e52 to ebdc149 Compare January 15, 2016 15:36
- adds `env` verb, which mimics the UNIX env command, but with the
  ability to read environment variables from stdin
- defines repro commands as the piping of the two calls:
  `cd CWD; catkin build --get-env PKG | catkin env -si CMD; cd -`
@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch from ebdc149 to e87f9ed Compare January 15, 2016 16:09
@wjwwood
Copy link
Member

wjwwood commented Jan 15, 2016

Should catkin build --get-env PKG return newline separated environment variables? Right now it seems to return whitespace separated entries.

@jbohren
Copy link
Contributor Author

jbohren commented Jan 16, 2016

Should catkin build --get-env PKG return newline separated environment variables? Right now it seems to return whitespace separated entries.

Yeah, that's how it's designed, but hold off on merging this for the moment.

@jbohren jbohren changed the title [PR196 1/3+] Making stage reproduction more portable [PR196 1/3+] Making stage reproduction more portable, making resultspace sourcing more robust Jan 16, 2016
@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch 5 times, most recently from 382bf1b to efde288 Compare January 16, 2016 19:22
@jbohren
Copy link
Contributor Author

jbohren commented Jan 17, 2016

@wjwwood This is good to merge, once you've tested it.

@jbohren
Copy link
Contributor Author

jbohren commented Jan 17, 2016

@wjwwood You can get a human-readable printout like this:

catkin build --get-env PKGNAME | catkin env -si

We could add another option to get a human readable output without the pipes, but I don't know how important that is at the moment.

@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch 6 times, most recently from 8084e60 to 71f1c09 Compare January 19, 2016 15:57
@jbohren
Copy link
Contributor Author

jbohren commented Jan 19, 2016

@wjwwood This should be good to review. It should be robust to the PTY exhaustion issues on OS X, and we can re-enable emulate_tty once there's a better solution for that (in osrf_pycommon, I presume).

It also uses an asynchronous lock for the install lock, and treats it as a generic "locked resource" for a given stage.

Also, I ran into the same problem with execute_process which caused it prematurely terminate before all output had been received in the resultspace loading (note here). Because of that I switched it back to using subprocess.Popen. I think this indicates another bug in the execute_process of osrf_pycommon.

@jbohren
Copy link
Contributor Author

jbohren commented Jan 19, 2016

Also, since catkin build --get-env emulates typeset -px, it prints out environment variables in a human-readable way.

jbohren and others added 4 commits January 19, 2016 15:47
execution: switching time.sleep to asyncio.sleep in executor coroutint
- uses `catkin env` to handle environment variables which contain line
  breaks
- makes `catkin env` flush stdout to make sure all output gets processed
  before the resultspace loading receives the return code
- properly escapes environment paths to load from
- reports errors more clearly
build: Clean up exit from build cli

osx: Setting notty on OS X to avoid exhausting PTYs, cleaning up install lock implementation

python3: decode popen result in resultspace loading

resultspace: adding note

stages: removing OS X nonfix
@jbohren jbohren force-pushed the pre-0.4.0-executor-env-verb branch from 185c94a to 14c5ce0 Compare January 19, 2016 20:47
@@ -348,6 +362,7 @@ def execute_jobs(
def run_until_complete(coroutine):
# Get event loop
loop = get_loop()
loop.slow_callback_duration = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to stay in? What callbacks do you expect might take longer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 100ms. Many callbacks take upwards of 300ms, which causes Trollius to generate warnings.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

wjwwood added a commit that referenced this pull request Jan 19, 2016
[PR196 1/3+] Making stage reproduction more portable, making resultspace sourcing more robust
@wjwwood wjwwood merged commit 87f7715 into pre-0.4.0-executor Jan 19, 2016
@wjwwood wjwwood deleted the pre-0.4.0-executor-env-verb branch January 19, 2016 23:24
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.

2 participants