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

absolutify --project path #28625

Merged
merged 1 commit into from
Aug 24, 2018

Conversation

fredrikekre
Copy link
Member

Fixes:

julia> Base.active_project()
"/home/fredrik/.julia/dev/Pkg/Project.toml"

julia> cd("Pkg")

julia> Base.active_project()
"/home/fredrik/.julia/dev/Pkg/Pkg/Project.toml"

note Pkg/Pkg in the second path.

@fredrikekre fredrikekre added bugfix This change fixes an existing bug backport pending 1.0 labels Aug 13, 2018
base/initdefs.jl Outdated
@@ -136,10 +136,10 @@ function init_load_path()
end
project = (JLOptions().project != C_NULL ?
unsafe_string(Base.JLOptions().project) :
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be handled in init.c, for conisistency with all other options that define paths

Copy link
Member Author

Choose a reason for hiding this comment

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

The abspath call? Like

julia/src/init.c

Lines 587 to 594 in 0846eb5

if (jl_options.outputo)
jl_options.outputo = abspath(jl_options.outputo, 0);
if (jl_options.outputji)
jl_options.outputji = abspath(jl_options.outputji, 0);
if (jl_options.outputbc)
jl_options.outputbc = abspath(jl_options.outputbc, 0);
if (jl_options.machine_file)
jl_options.machine_file = abspath(jl_options.machine_file, 0);
?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need the abspath here for relative paths from JULIA_PROJECT though.

@StefanKarpinski
Copy link
Member

julia> Base.active_project()
"/home/fredrik/.julia/dev/Pkg/Project.toml"

How did you start Julia to get this?

base/initdefs.jl Outdated
HOME_PROJECT[] =
project == "" ? nothing :
project == "@." ? current_project() : project
project == "@." ? current_project() : abspath(project)
Copy link
Member

Choose a reason for hiding this comment

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

If we resolve this at startup time, you can't do --project=. and have it follow you around. I'm not sure if resolving it at startup or having it follow you around is better.

Copy link
Member

@KristofferC KristofferC Aug 13, 2018

Choose a reason for hiding this comment

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

Following around to the exact pwd is something I think you rarely want.

➜  Pkg git:(kc/free_repo) ✗ ~/julia/julia --project=.
(Pkg) pkg>

shell> cd src/
/Users/kristoffer/.julia/dev/Pkg/src

(src) pkg>

What you might want is to have the same behavior as we had before where you try to find the project based on your current path by looking in parent folders, but we wanted to write that as @.. or something?

We tried to move away from pwd being important so I think abspath here is definitely right.

Copy link
Member

@KristofferC KristofferC Aug 13, 2018

Choose a reason for hiding this comment

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

@fredrikekre could just change this to project === nothing ? nothing : abspath(project) to fix Stefans comment?

Copy link
Member

Choose a reason for hiding this comment

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

There's no way to do that from the command-line or environment, but you can accomplish it now by doing Base.HOME_PROJECT[] = "@." to circumvent the startup time expansion and delay it until load time.

base/initdefs.jl Outdated
@@ -136,10 +136,10 @@ function init_load_path()
end
project = (JLOptions().project != C_NULL ?
unsafe_string(Base.JLOptions().project) :
get(ENV, "JULIA_PROJECT", nothing))
get(ENV, "JULIA_PROJECT", ""))
Copy link
Member

Choose a reason for hiding this comment

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

This has a behavioral side effect: it makes setting JULIA_PROJECT to the empty string behave the same as having it unset, which doesn't seem ideal and is something that I've generally avoided.

@fredrikekre
Copy link
Member Author

How did you start Julia to get this?

julia-master --project=Pkg

If we resolve this at startup time, you can't do --project=. and have it follow you around. I'm not sure if resolving it at startup or having it follow you around is better.

I agree with Kristoffer here. The reason I found this is that I made a doc-build project with needed deps, but Documenter cds to the build directory, so when I tried to load packages from doctests I got an error, since the env had changed.

There's no way to do that from the command-line or environment, but you can accomplish it now by doing Base.HOME_PROJECT[] = "@."

We can skip the abspath call in init_load_path and only keep the one for the command line flag in init.c. That way you can set JULIA_PROJECT=@. to get that behaviour.

@fredrikekre fredrikekre changed the title absolutify --project and JULIA_PROJECT paths absolutify --project path Aug 14, 2018
@fredrikekre
Copy link
Member Author

I changed this to only absolutify --project paths. That is now consistent with how we treat other command line arguments that accepts paths. I left JULIA_PROJECT alone; we don't call abspath for other environment variables, so that is consistent. Also, I think it makes sense to abspath --project but not JULIA_PROJECT since --project paths are much more local to the process that you start, environment variables should "work" wherever you start julia, and hence it is expected that those are set as absolute paths from the user. Thoughts?

@fredrikekre fredrikekre force-pushed the fe/absolute-project branch 2 times, most recently from b54b46a to fc5324d Compare August 14, 2018 11:15
@StefanKarpinski
Copy link
Member

We can skip the abspath call in init_load_path and only keep the one for the command line flag in init.c. That way you can set JULIA_PROJECT=@. to get that behaviour.

That's not what everyone thought was the better default when I was working on that.

@fredrikekre
Copy link
Member Author

Sorry, I was answering this:

If we resolve this at startup time, you can't do --project=. and have it follow you around.

Which you can still accomplish with JULIA_PROJECT=. if you want.

@StefanKarpinski
Copy link
Member

Which you can still accomplish with JULIA_PROJECT=. if you want.

No, they'd both be resolved at startup time. You'd have to do Base.HOME_PROJECT[] = ".".

@fredrikekre
Copy link
Member Author

No, they'd both be resolved at startup time.

Not in the state the PR is now.

@StefanKarpinski
Copy link
Member

I don't think that changing the way the command-line option and the environment variable behave is a great idea. That seems like it will be a huge source of confusion.

@KristofferC KristofferC mentioned this pull request Aug 19, 2018
@KristofferC
Copy link
Member

Bump, would be nice to have this bug fixed in 1.0.1. What can we do to make progress here?

@fredrikekre
Copy link
Member Author

What can we do to make progress here?

Merge? :trollface:

So the problem I have with the current behaviour is that the environment I explicitly choose to start julia with is changed under my feet, by code that does not do any Pkg operations (only cd). I understand that I maybe can't control this if the code explicitly changes environment, but I feel like environment unaware code should not change which code I will load.

I guess the question is if we should do the same for JULIA_PROJECT (which was in here from the beginning), I think that might be good to add back maybe.

@KristofferC
Copy link
Member

Yes, unless we are explicit we don't want to accidentally activate random environments by changing pwd (which is a security concern).

@StefanKarpinski, do you have any issue with the current implementation? Barring comments, I will merge this in a couple of days.

fredrikekre added a commit to Ferrite-FEM/Ferrite.jl that referenced this pull request Aug 22, 2018
fredrikekre added a commit to Ferrite-FEM/Ferrite.jl that referenced this pull request Aug 22, 2018
fredrikekre added a commit to Ferrite-FEM/Ferrite.jl that referenced this pull request Aug 22, 2018
* Use projects for doc build and code coverage submission.

* workaround JuliaLang/julia/pull/28625
@fredrikekre
Copy link
Member Author

fredrikekre commented Aug 23, 2018

I added back the absolutification of JULIA_PROJECT, so now JULIA_PROJECT and --project behaves the same. In the rare case that one wants to make the project follow pwd(), then it is possible to Base.HOME_PROJECT[] = ".".

@StefanKarpinski
Copy link
Member

That makes me much happier with this :)

@fredrikekre fredrikekre merged commit eb8a933 into JuliaLang:master Aug 24, 2018
@fredrikekre fredrikekre deleted the fe/absolute-project branch August 24, 2018 19:12
@@ -593,6 +593,8 @@ static void jl_resolve_sysimg_location(JL_IMAGE_SEARCH rel)
jl_options.outputbc = abspath(jl_options.outputbc, 0);
if (jl_options.machine_file)
jl_options.machine_file = abspath(jl_options.machine_file, 0);
if (jl_options.project && strncmp(jl_options.project, "@.", strlen(jl_options.project)) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

can use strcmp – this would just be a more verbose and slower way of implementing that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants