Respect startup-file flag in Pkg.build and Pkg.test #22094

omus · 2017-05-27T00:23:53Z

Fixes #21823 since Pkg.update calls Pkg.build

tkelman · 2017-05-27T00:37:58Z

probably worth adding a startup_file kwarg so you could turn this back on if you wanted (defaulting to off is better though)

yuyichao

This does not fix #21823 since it still doesn't "respect --startup-file=no" .

omus · 2017-05-29T13:39:35Z

I'll add @tkelman's suggestion to support loading the startup file through a keyword.

omus · 2017-05-29T13:53:15Z

After thinking about it more I think think we shouldn't support a startup_file keyword. I don't think either Pkg.test or Pkg.build should depend on anything within the startup file since this would be a requirement which is not properly tracked by the package. Additionally, having the startup_file keyword exposes that both Pkg.test and Pkg.build are launching new Julia processes which may not be true in the future.

omus · 2017-05-29T13:55:05Z

This does not fix #21823 since it still doesn't "respect --startup-file=no" .

Although the title of the issue wants Pkg.update to respect the startup-file flag all the OP is asking for is that his startup-file is not loaded during Pkg.update. I don't think there is a good use case for loading the startup-file here.

tkelman · 2017-05-29T13:56:49Z

I'm thinking about deployments in unusual environments that may be using juliarc to set up custom library paths, that kind of thing. I wouldn't think it would be a commonly used flag, but I don't see an issue with exposing that separate processes are getting launched. If that changes we can deprecate the keyword arguments - we already have one for code coverage which is a similar situation.

omus · 2017-05-29T14:33:16Z

I'm thinking about deployments in unusual environments that may be using juliarc to set up custom library paths, that kind of thing.

A good point. Since several Pkg functions eventually call Pkg.build (like Pkg.update) we would also need to have the keyword available for those functions as well.

It seems that the easiest solution would be to just respect the --startup-file flag from the parent Julia process. Unfortunately this means the default will be to load the startup-file but at least users will have the option to disable it.

omus · 2017-05-29T22:02:42Z

The JLOptions().startupfile uses 0 for unset, 1 for "yes", and 2 for "off".

tkelman · 2017-05-30T10:09:44Z

This version seems like an improvement over what we've got now at least (and would make a little more sense to consider backporting even, as opposed to new keyword args). Could use a test though.

omus · 2017-05-30T14:08:06Z

Could use a test though.

Is it safe to temporarily modify abspath(JULIA_HOME,"..","etc","julia","juliarc.jl") for a test?

tkelman · 2017-05-30T14:39:34Z

could set HOME in a withenv ?

tkelman · 2017-06-05T07:27:24Z

base/pkg/entry.jl

@@ -719,7 +720,7 @@ function test!(pkg::AbstractString,
                    --compilecache=$(Bool(Base.JLOptions().use_compilecache) ? "yes" : "no")
                    --check-bounds=yes
                    --startup-file=$(Base.JLOptions().startupfile != 2 ? "yes" : "no")
-                    $test_path
+                    --eval $code


why is this change necessary?

Not strictly necessary but evalfile does make the "runtests.jl" file run inside of a module which means variables defined within the startup-file can only be accessed via Main.*. I made this modification to make it harder to accidentally reference a variable within the tests which was defined or imported by the startup-file.

that did necessitate the messy escape_string business though

is running the runtests.jl inside a module via evalfile going to result in any packages' tests behaving differently?

tkelman · 2017-06-05T19:48:30Z

the appveyor timeouts look like redirection to a file was necessary

omus · 2017-06-06T14:14:22Z

could set HOME in a withenv?

Looks like for Windows you need to set USERPROFILE.

omus · 2017-06-06T23:12:38Z

I'll try to sort out the issues tomorrow

Issue fixed.

Flags given to subprocesses are now alphabetized.

omus · 2017-06-07T20:32:48Z

Resolved the following issues:

Needed to use escape_string in Pkg.test to work correctly with Windows paths
isdefined(::Symbol) was deprecated which caused my tests to fail on Linux and Mac due to starting julia with --depwarn=error

omus · 2017-06-08T13:33:47Z

Ready to merge

omus · 2017-06-09T15:02:15Z

I'll merge this today unless there are objections. I need to close one of the 1800 issues ;)

tkelman · 2017-06-09T15:42:38Z

had a now-collapsed question on the change to using evalfile

is running the runtests.jl inside a module via evalfile going to result in any packages' tests behaving differently?

I guess we'll find out, but I'm a bit concerned if packages will notice the change.

omus · 2017-06-09T16:17:28Z

is running the runtests.jl inside a module via evalfile going to result in any packages' tests behaving differently?

Packages which assume the tests are running in the Main module could break. For example Compat.jl will run require two changes in runtests.jl lines 1570 and 1855. I'll make a Compat.jl PR.

omus · 2017-06-09T16:20:34Z

I'll make a NEWS entry to make debugging this change easier.

tkelman · 2017-06-09T18:06:06Z

In that case I don't personally think this (edit: just the evalfile part) behavior change to Pkg.test is worth the disruption - CI will virtually never have a juliarc, after all.

iamed2 · 2017-06-09T18:09:16Z

@tkelman Are you referring to just the evalfile part?

iamed2 · 2017-06-09T18:14:38Z

Package tests really shouldn't be assuming Main, especially since current_module()/@__MODULE__ is available.

tkelman · 2017-06-09T18:20:26Z

Are you referring to just the evalfile part?

Right, the change in which module things run in (the --startup-file part is worth doing). Package tests have been run in the existing way for as long as Pkg.test has existed, and tweaking the module doesn't seem to be accomplishing much of anything aside from this minor point about juliarc isolation.

omus · 2017-06-09T18:30:45Z

I'd like to get the juliarc isolation included but I can accept that it is a controversial change. I'll remove that change from this PR so the remainder of the changes can at least be merged.

tkelman · 2017-06-09T18:35:10Z

Sorry, yes, propagating --startup-file is a definite improvement and not disruptive, but is independent of whether or not Pkg.test runs code via evalfile

tkelman · 2017-06-09T19:38:04Z

LGTM now, though the NEWS bit should move to where the evalfile change goes

omus · 2017-06-09T19:38:34Z

LGTM now, though the NEWS bit should move to where the evalfile change goes

Thanks. I just noticed that myself.

tkelman

+1 to merge if CI is green

omus mentioned this pull request May 27, 2017

Assign PROGRAM_FILE earlier #22092

Merged

kshyatt added the packages Package management and loading label May 27, 2017

yuyichao previously requested changes May 27, 2017

View reviewed changes

omus force-pushed the cv/pkg-startup branch from 7e33bd9 to 582662c Compare May 29, 2017 14:41

omus changed the title ~~Disable loading .juliarc.jl for Pkg.build and Pkg.test~~ Respect startup-file flag in Pkg.build and Pkg.test May 29, 2017

omus force-pushed the cv/pkg-startup branch from 582662c to d85bed4 Compare May 29, 2017 22:03

tkelman reviewed Jun 5, 2017

View reviewed changes

omus force-pushed the cv/pkg-startup branch from 7c63665 to ba325ed Compare June 5, 2017 14:30

omus force-pushed the cv/pkg-startup branch 2 times, most recently from 33bb3ce to 6785184 Compare June 6, 2017 19:36

Standardize Pkg.build/Pkg.test flag order

6836f75

Flags given to subprocesses are now alphabetized.

omus force-pushed the cv/pkg-startup branch 2 times, most recently from 605c4d3 to e235353 Compare June 7, 2017 20:30

omus mentioned this pull request Jun 9, 2017

Don't assume tests are running in Main JuliaLang/Compat.jl#364

Merged

omus force-pushed the cv/pkg-startup branch from dac0d80 to 93b7ca5 Compare June 9, 2017 19:34

Respect startup-file during Pkg.build and Pkg.test

4f127f4

omus force-pushed the cv/pkg-startup branch from 93b7ca5 to 4f127f4 Compare June 9, 2017 19:43

tkelman approved these changes Jun 9, 2017

View reviewed changes

tkelman merged commit f987466 into master Jun 9, 2017

tkelman deleted the cv/pkg-startup branch June 9, 2017 23:54

omus mentioned this pull request Jun 10, 2017

Make Pkg.test runs tests within an anonymous module #22328

Closed

fredrikekre mentioned this pull request Aug 18, 2017

"--startup-file=no" should also apply for spawned Julia processes #23318

Closed

fredrikekre mentioned this pull request Jun 27, 2018

should test load startup.jl? JuliaLang/Pkg.jl#435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect startup-file flag in Pkg.build and Pkg.test #22094

Respect startup-file flag in Pkg.build and Pkg.test #22094

omus commented May 27, 2017

tkelman commented May 27, 2017

yuyichao left a comment

omus commented May 29, 2017

omus commented May 29, 2017

omus commented May 29, 2017

tkelman commented May 29, 2017

omus commented May 29, 2017 •

edited

Loading

omus commented May 29, 2017 •

edited

Loading

tkelman commented May 30, 2017

omus commented May 30, 2017

tkelman commented May 30, 2017

tkelman Jun 5, 2017

omus Jun 5, 2017 •

edited

Loading

tkelman Jun 7, 2017

tkelman Jun 8, 2017

tkelman commented Jun 5, 2017

omus commented Jun 6, 2017

omus commented Jun 6, 2017

omus commented Jun 7, 2017

omus commented Jun 8, 2017

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017

omus commented Jun 9, 2017 •

edited

Loading

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017 •

edited

Loading

iamed2 commented Jun 9, 2017

iamed2 commented Jun 9, 2017 •

edited

Loading

tkelman commented Jun 9, 2017 •

edited

Loading

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017 •

edited

Loading

tkelman commented Jun 9, 2017

omus commented Jun 9, 2017 •

edited

Loading

tkelman left a comment

Respect startup-file flag in Pkg.build and Pkg.test #22094

Respect startup-file flag in Pkg.build and Pkg.test #22094

Conversation

omus commented May 27, 2017

tkelman commented May 27, 2017

yuyichao left a comment

Choose a reason for hiding this comment

omus commented May 29, 2017

omus commented May 29, 2017

omus commented May 29, 2017

tkelman commented May 29, 2017

omus commented May 29, 2017 • edited Loading

omus commented May 29, 2017 • edited Loading

tkelman commented May 30, 2017

omus commented May 30, 2017

tkelman commented May 30, 2017

tkelman Jun 5, 2017

Choose a reason for hiding this comment

omus Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

tkelman Jun 7, 2017

Choose a reason for hiding this comment

tkelman Jun 8, 2017

Choose a reason for hiding this comment

tkelman commented Jun 5, 2017

omus commented Jun 6, 2017

omus commented Jun 6, 2017

omus commented Jun 7, 2017

omus commented Jun 8, 2017

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017

omus commented Jun 9, 2017 • edited Loading

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017 • edited Loading

iamed2 commented Jun 9, 2017

iamed2 commented Jun 9, 2017 • edited Loading

tkelman commented Jun 9, 2017 • edited Loading

omus commented Jun 9, 2017

tkelman commented Jun 9, 2017 • edited Loading

tkelman commented Jun 9, 2017

omus commented Jun 9, 2017 • edited Loading

tkelman left a comment

Choose a reason for hiding this comment

omus commented May 29, 2017 •

edited

Loading

omus commented May 29, 2017 •

edited

Loading

omus Jun 5, 2017 •

edited

Loading

omus commented Jun 9, 2017 •

edited

Loading

tkelman commented Jun 9, 2017 •

edited

Loading

iamed2 commented Jun 9, 2017 •

edited

Loading

tkelman commented Jun 9, 2017 •

edited

Loading

tkelman commented Jun 9, 2017 •

edited

Loading

omus commented Jun 9, 2017 •

edited

Loading