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

Exit calls in build scripts no longer build to abort #20070

Merged
merged 4 commits into from
Apr 16, 2017

Conversation

omus
Copy link
Member

@omus omus commented Jan 16, 2017

Captures and "ignores" exit calls made within build.jl scripts.

Closes #20020

@yuyichao
Copy link
Contributor

Continuing in atexit sounds like a bad idea. Especially as a hack to workaround package bug.

@omus
Copy link
Member Author

omus commented Jan 16, 2017

No disagreements about it being a hack. That said I don't think that a package bug should be allowed to interrupt the build process in base.

@yuyichao
Copy link
Contributor

That said I don't think that a package bug should be allowed to interrupt the build process in base.

You can do that no matter what.

@omus
Copy link
Member Author

omus commented Jan 16, 2017

As @tkelman mentioned #20020 (comment)

Looks like this was an unforeseen consequence of the difference between #13499 vs #13506, where at the time I was concerned about Julia process startup time on Windows. Since that's fixed now and much less of a concern, does seem like it would be safer to do one process per package.

I can update the PR to spawn multiple processes if that is preferable.

@kshyatt kshyatt added the building Build system, or building Julia or its dependencies label Jan 16, 2017
@tkelman tkelman added packages Package management and loading and removed building Build system, or building Julia or its dependencies labels Jan 16, 2017
@omus
Copy link
Member Author

omus commented Jan 25, 2017

Revised the PR to spawn a new Julia process for each package build script. I should probably do some additional work on how the exceptions are being captured.

@tkelman
Copy link
Contributor

tkelman commented Apr 5, 2017

This is still a worthwhile change, if you or anyone else has capacity to get it passing.

@omus
Copy link
Member Author

omus commented Apr 14, 2017

Resolved conflicts.

@tkelman
Copy link
Contributor

tkelman commented Apr 14, 2017

Nice, looks like it's working. This should theoretically not be user-visible, but will make Pkg.build a bit more robust to packages influencing one another. cc @stevengj if you're able to take a look

@stevengj
Copy link
Member

Why is julia spawning time no longer an issue on Windows?

@tkelman
Copy link
Contributor

tkelman commented Apr 14, 2017

#17179

@tkelman
Copy link
Contributor

tkelman commented Apr 15, 2017

@omus would the intermediate commits pass here or should this be squashed?

@omus
Copy link
Member Author

omus commented Apr 15, 2017

All intermediate commits pass locally.

@tkelman tkelman merged commit eee75d1 into JuliaLang:master Apr 16, 2017
@omus omus deleted the build-exit branch April 16, 2017 00:38
--eval $code
```

success(pipeline(cmd, stderr=STDERR))
Copy link
Member

Choose a reason for hiding this comment

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

why we do not resend stdout to STDOUT?

Copy link
Member

@iblislin iblislin Sep 6, 2017

Choose a reason for hiding this comment

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

MXNet.jl ran into this: dmlc/MXNet.jl#265 (comment)

We need to build libmxnet via make. The main build step requires more than 10 mins on travis-ci. But stdout is gone in Julia 0.6+, so the build got killed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely an oversight. We should add a test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants