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

Support shared named environments during startup #40025

Merged
merged 4 commits into from
Mar 24, 2021

Conversation

oschulz
Copy link
Contributor

@oschulz oschulz commented Mar 14, 2021

Implements #35354 .

I hope I did this right - from what I saw, most things @fredrikekre had in his proposed patch in #35354, load_path_expand() does already now. So just using load_path_expand() instead of expanduser() in init_active_project() seems to do the trick. I'm not an expert on Julia's init process, though.

WIth this, both --project=@myenv and JULIA_PROJECT=@myenv seem to behave the same way as activate @myenv on the Pkg console.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 14, 2021

Ah, not quite that simple it seems (see failed tests), fixing that now.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 15, 2021

Got tests to pass - @vtjnash , are the additional tests sufficient (sorry if I got the wrong guy, GitHub suggested you as reviewer)?

base/initdefs.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 15, 2021
Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

This doesn't work on Windows? I see no reason it shouldn't, and in that case the new tests should be moved out from the if.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 15, 2021

Good point - why are the previous test cases in if !Sys.iswindows(), though?

@fredrikekre
Copy link
Member

Because they test expansion of ~.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 15, 2021

Ahh, silly me. Ok, I'll move it out.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 16, 2021

Ok, tests are much nicer now.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 16, 2021

Question: load_path_expand(project) returns the full project path including "/Project.toml" and expanduser(project) just returns the directory. Is that Ok (same mixed behavior as before) or should we harmonize this so that init_active_project() always returns the project with or without "/Project.toml" at the end?

@oschulz
Copy link
Contributor Author

oschulz commented Mar 18, 2021

@vtjnash , is this good to merge as is, for you?

@fredrikekre
Copy link
Member

LGTM, but might be news-worthy?

@oschulz
Copy link
Contributor Author

oschulz commented Mar 18, 2021

Good point - under "Command-line option changes", I guess?

@oschulz
Copy link
Contributor Author

oschulz commented Mar 20, 2021

I added a news item - text Ok like that?

@musm
Copy link
Contributor

musm commented Mar 24, 2021

@oschulz Was the force-push just a documentation change? I want to confirm I can merge this if any code-changes were made after the approvals.

@oschulz
Copy link
Contributor Author

oschulz commented Mar 24, 2021

@oschulz Was the force-push just a documentation change?

Oh, sorry - just wanted to present a cleaner history. No, the force push included improvements to the tests that I added after @fredrikekre 's initial approval (since he suggested to also test on Windows).

@musm musm merged commit 10ab32f into JuliaLang:master Mar 24, 2021
@oschulz oschulz deleted the shared-env-startup branch March 24, 2021 18:47
@oschulz
Copy link
Contributor Author

oschulz commented Mar 24, 2021

Thanks!

@oschulz
Copy link
Contributor Author

oschulz commented Mar 24, 2021

I should have done this a few weeks earlier, in time for v1.6. :-)

@musm
Copy link
Contributor

musm commented Mar 24, 2021

That gives you a reason to look forward to 1.7 😉

@oschulz
Copy link
Contributor Author

oschulz commented Mar 24, 2021

;-)

@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* Support shared named envs during startup

* Improved handling of named envs during startup

Co-authored-by: Jameson Nash <[email protected]>

* Improve tests for --project and JULIA_PROJECT

* Add news item about shared env support at startup

Co-authored-by: Jameson Nash <[email protected]>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* Support shared named envs during startup

* Improved handling of named envs during startup

Co-authored-by: Jameson Nash <[email protected]>

* Improve tests for --project and JULIA_PROJECT

* Add news item about shared env support at startup

Co-authored-by: Jameson Nash <[email protected]>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
* Support shared named envs during startup

* Improved handling of named envs during startup

Co-authored-by: Jameson Nash <[email protected]>

* Improve tests for --project and JULIA_PROJECT

* Add news item about shared env support at startup

Co-authored-by: Jameson Nash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants