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

eval build.jl files in a separate Julia process for Pkg.build #13506

Merged
merged 1 commit into from
Oct 9, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Oct 9, 2015

This fixes #13458.

It supersedes #13499, which spawned a separate process for each build.jl file. Instead, as requested by @tkelman (since launching julia is slow on Windows), my new version spawns a single Julia process which evals all the build.jl files. This should suffice to fix the problems described in #13458 — it isolates the build from the running Julia process, and since packages are built in dependency order it should be okay that their builds share a process.

@stevengj stevengj added the packages Package management and loading label Oct 9, 2015
end
"""
io, pobj = open(detach(`$(Base.julia_cmd())
--startup-file=no --history-file=no
Copy link
Contributor

Choose a reason for hiding this comment

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

there might be some env vars or other settings used to customize build behavior that people might put in their juliarc...

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, although I generally think it's a good idea for packages to "remember" the settings from the last build (see how IJulia handles this with a deps/JUPYTER file) so that things don't change unexpectedly on Pkg.update() if the user forgot to set an environment variable the same way as when they added the package...

Copy link
Contributor

Choose a reason for hiding this comment

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

You're probably right there. Socially-enforced/demonstrated best practices will hopefully get us there over time, maybe keep in mind and suggest something more built-in and uniform for future revisions of Pkg.

@stevengj
Copy link
Member Author

stevengj commented Oct 9, 2015

@wildart, @StefanKarpinski, looks good to you?

@wildart
Copy link
Member

wildart commented Oct 9, 2015

lgtm

stevengj added a commit that referenced this pull request Oct 9, 2015
eval build.jl files in a separate Julia process for Pkg.build
@stevengj stevengj merged commit 6f3ccdd into JuliaLang:master Oct 9, 2015
@stevengj stevengj deleted the buildspawn1 branch October 9, 2015 20:47
@stevengj
Copy link
Member Author

stevengj commented Oct 9, 2015

@tkelman, okay to backport?

@stevengj
Copy link
Member Author

stevengj commented Oct 9, 2015

(Once we've kicked the tires on this in 0.5 a bit.)

@tkelman
Copy link
Contributor

tkelman commented Oct 9, 2015

Would like to be cautious on this one. Have you tested it at all on windows yet? I don't think the pkg tests exercise build.

@stevengj
Copy link
Member Author

No, I haven't tried it on Windows; I tested it locally on my MacOS machine with both successful and failing builds, but I didn't realize that the Pkg tests don't exercise Pkg.build.

@stevengj
Copy link
Member Author

I agree that it is not something that should be backported right away, but it is hopefully something that we can backport eventually once it has gotten more testing by 0.5 developers. Otherwise I think we will see a stream of people complaining that Pkg.update() is giving them inexplicable build failures.

@tkelman
Copy link
Contributor

tkelman commented Oct 10, 2015

Yeah, winrpm started segfaulting and I suspect it was this PR:

https://ci.appveyor.com/project/tkelman/lightxml-jl/branch/master

@malmaud
Copy link
Contributor

malmaud commented Oct 17, 2015

Local testing points to this as responsible for JuliaPackaging/Homebrew.jl#98

@stevengj
Copy link
Member Author

I just tried Pkg.update(); Pkg.build("Homebrew") on my Mac, and it seemed to go fine. What am I missing? Do I need the master version of Homebrew or something?

@stevengj
Copy link
Member Author

Hmm, probably we need to pass the load path etc. to the subprocess as in the compilecache process.

@tkelman
Copy link
Contributor

tkelman commented Oct 20, 2015

The WinRPM issue may have been resolved by something else between failing at 03f28fd (https://ci.appveyor.com/project/tkelman/lightxml-jl/build/1.0.116/job/mxfy3r046ty2dyc6) and passing at 8a86270 (https://ci.appveyor.com/project/tkelman/lightxml-jl/build/1.0.117/job/i7mgacf6jx78s2ou). Worth looking into what it was (edit: #13553 was the fix, apparently - will try to verify the initial cause)

@malmaud can you test if #13689 fixes the homebrew issue locally?

@malmaud
Copy link
Contributor

malmaud commented Oct 22, 2015

Not quite sure why, but just switching Homebrew to use pipeline instead of the deprecated run syntax for all its external program invocations has fixed the issue (at least locally). Remains to be seen if it also fixes Homebrew installation on Travis.

@tkelman
Copy link
Contributor

tkelman commented Oct 23, 2015

And I tracked down the WinRPM issue to having been caused by #13491, which makes sense given that #13553 was the fix. d3af973 worked https://ci.appveyor.com/project/tkelman/lightxml-jl/build/1.0.126/job/22w3m10o2tdu806w, but 90bce78 failed https://ci.appveyor.com/project/tkelman/lightxml-jl/build/1.0.126/job/8c7j76tfvl0q8rlj. There's a very small chance it was actually #13498 that caused the problem, but I doubt it.

I feel like the Homebrew problem is indicative of a bug somewhere in base that should be tracked down, but at least for now this looks safe again. Sorry for blaming the wrong PR!

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.

Pkg.build should launch a separate Julia process?
4 participants