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

Set temporary env whenever Julia is started with --project=@temp #51149

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

ven-k
Copy link
Member

@ven-k ven-k commented Sep 1, 2023

This makes it convenient to start and run Julia in a temporary environment. This brings Pkg.activate(; temp=true) functionality to cmdline.

  • Update relevant docs.
  • Add tests for --project=@temp and JULIA_PROJECT="@temp".

@vtjnash
Copy link
Member

vtjnash commented Sep 1, 2023

Similar to #49061 (@KristofferC), and discussion there of whether we should spell this --temp to exactly match the Pkg REPL behavior

@fingolfin
Copy link
Member

This could breaks existing scripts, though. How about using @temp (similar to how there is @.)

@ericphanson
Copy link
Contributor

#50864 uses @ as well

@ven-k ven-k changed the title Set temporary env whenever Julia is started with --project=temp Set temporary env whenever Julia is started with --project=--temp Sep 2, 2023
@ven-k ven-k force-pushed the vkb/temp-project branch 2 times, most recently from 2a981b8 to b7663be Compare September 2, 2023 08:05
@fingolfin
Copy link
Member

You have now changed it to --project=--temp. That's pretty unusual and fits into no other pattern, does it? Could you at least explain your rationale over the suggestion to use @temp, which would by the way also naturally fit in with the code in load_path_expand

@ven-k
Copy link
Member Author

ven-k commented Sep 2, 2023

The @abc pattern is already used to activate shared env abc in the envdir(). So, avoiding @temp might be better.

Although --project=--temp looks unusual, it follows the ] activate --temp pattern.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I find that argument compelling. Should this have a NEWS.md entry perhaps?

@vtjnash vtjnash added the needs news A NEWS entry is required for this change label Feb 10, 2024
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Yes, if someone could type a blurb for the NEWS file, this looks good to me

@vtjnash vtjnash added the forget me not PRs that one wants to make sure aren't forgotten label Feb 10, 2024
@vtjnash vtjnash added this to the 1.11 milestone Feb 10, 2024
@adienes
Copy link
Member

adienes commented Feb 10, 2024

I really appreciate the functionality but sorry must it be spelled --project=--temp ? I can't think of a single other CLI app I've ever used with the pattern --arg1=--arg2. I understand that within Pkg it is ] activate --temp, but in this context it just seems extremely odd.

I much prefer the original --project=@temp (which as previously noted mirrors the new --project=@scriptdir) or something independent like --temp-project

@vtjnash
Copy link
Member

vtjnash commented Feb 10, 2024

can't think of a single other CLI app I've ever used

It can be a somewhat common pattern in compiler drivers like Julia, gcc, or clang. E.g. gcc -Wl,-z,undef or clang -mllvm

@ven-k
Copy link
Member Author

ven-k commented Feb 11, 2024

As conversations, here and in #49061, have gravitated toward --project=--temp, I've retained that format.

--temp-project looks very neat; but I'm not sure if a new argument that is a subset of another, would be a good idea.

@adienes
Copy link
Member

adienes commented Feb 11, 2024

reading through both these issue threads, I would not call it "gravitating," more just stalled after --temp was proposed. Just counting heads, I definitely see more contributors proposing or preferring other options to --temp (by my count of the thumb wars, it's 6-3). and the suggestion for --project=_ has 6x👍 from non-commenters

that being said, OSS is not a vote, and I am happy to be overruled. just wanted to put my feedback in that I find this syntax quite clunky.

@vtjnash that's a good point about the compiler drivers, but I strongly suspect that most users see Julia more like an app than they see it like a compiler

@vtjnash vtjnash added triage This should be discussed on a triage call and removed needs news A NEWS entry is required for this change labels Feb 12, 2024
@Keno
Copy link
Member

Keno commented Feb 12, 2024

--temp is technically a legal directory name also though. I think the whole flags vs file heuristics that clis do is a bit of an antipattern (e.g. why there's often things like -- to separate them). I don't think it's any better than --project-temp though and I also think --project=@temp is fine (since @ is already special; yes I know that it currently means the temp shared environment - I don't think it's a big issue, but if people are concerned, it could only have the new meaning if the temp shared environment doesn't exist).

@mkitti
Copy link
Contributor

mkitti commented Feb 12, 2024

How about an independent command line option? You would invoke this as julia --temp. It is an error to provide both --project and --temp. I could go for --temp-project or --project-temp as well. Having the value of an option use a -- prefix looks strange to me and might even confuse some parsers.

@IanButterworth
Copy link
Member

IanButterworth commented Feb 12, 2024

--temp-project has the benefit that you could just type --temp because julia args only need to be complete enough to be unambiguous

@Moelf
Copy link
Contributor

Moelf commented Feb 12, 2024

in favor of --temp/--temp-project because @temp should be persistent just like any other @abc and --arg1=--arg2 is weird among all the other command-line interface.

@vtjnash
Copy link
Member

vtjnash commented Feb 12, 2024

--temp is technically a legal directory name also though

Just as an observation for everyone, the pattern to force it to be a directory is to prepend ./ to make it --project=./--temp if you really must use a relative path to a folder of that name. Same with @ being alternatively specified as --project=@./temp or --project=./@temp to force alternative meanings

@jishnub
Copy link
Contributor

jishnub commented Feb 12, 2024

Personally, I would prefer if this flag starts with --project so that it is easy to club it with the other forms in which --project= is used. From this perspective, I would prefer --project=@temp as it's the most visually similar form, but --project-temp also looks fine. --temp-project departs from this pattern, and even though it's easier to read than --project-temp, I would prefer consistency. The shortened form julia --temp looks cryptic to me, as it's unclear that the temp refers to the project.

@GunnarFarneback
Copy link
Contributor

Whatever is chosen here, please make it easy and convenient to use. Being able to write julia --temp is by far the best option I've seen suggested in this respect.

(As a side note, on international keyboards @ tends to be tucked away, e.g. as AltGr+2 on my keyboard.)

@adienes
Copy link
Member

adienes commented Feb 12, 2024

@temp should be persistent just like any other @abc

persistent-ly empty, so it kind of is

something is going to have to be special-cased, and I think it's least surprising if that's behind an @ symbol --- the universal Julian mark of "non-standard stuff be here"

and I know it has been mentioned a few times so this is a redundant observation, but nonetheless I think it's going unappreciated that --project=@scriptdir has been already chosen to special case an @keyword project to be something other than a persistent shared environment

@mkitti
Copy link
Contributor

mkitti commented Feb 12, 2024

If we use @temp I suggest that we should check if a shared environment named temp exists. We can then either

  1. Issue a warning and then just use the existing shared directory (backwards compatibility).
  2. Issue an error.

I'm learning towards option 1. It would be similar to a deprecation warning.

Additionally, we could offer a helpful error message about referring to the shared environment using its absolute path.

@Keno
Copy link
Member

Keno commented Feb 12, 2024

Oh, I also don't like the name @scriptdir much and totally missed the discussion of it. I would prefer @tecosaur's proposal of @script that behaves like @. looking for a project directory in the ancestors of the path to the currently running script file.

@scriptdir is new in 1.11. If you feel strongly, this can still be changed.

@ven-k ven-k force-pushed the vkb/temp-project branch 2 times, most recently from 5a0f8a4 to f8e7121 Compare February 13, 2024 19:57
@ven-k
Copy link
Member Author

ven-k commented Feb 13, 2024

With the update,

  • --project=@temp now starts Julia with a temp environment.
  • LOAD_PATH is now ["@", "@temp", "@v#.#", "@stdlib"]. This also means the o in y/n/o gives a temporary env as an option (follows the order in LOAD_PATH).

However it does not check for a shared env temp yet.

👍 I propose that we give @temp only one special meaning and not allow any shared env named temp.
To fully ensure this, Pkg should treat @temp, @stdlib and @. differently.

Current behavior:

(@v1.11) pkg> activate @.
ERROR: not a valid name for a shared environment: .

(@v1.11) pkg> activate @temp
  Activating new project at `~/.julia/environments/temp`

(@temp) pkg> activate @stdlib
  Activating new project at `~/.julia/environments/stdlib`

(@stdlib) pkg> activate @
  Activating new project at `~/.julia/environments`

(@environments) pkg> 

To new behavior:

(@v1.11) pkg> activate @.
# activate the current project

# While `--temp` continues to do what it does,
(@v1.11) pkg> activate @temp
  Activating new project at `/tmp/jl_oqUHmk`

(@temp) pkg> activate @stdlib
  Activating project at `@stdlib/v1.11`

(@stdlib) pkg> activate @
# activate the active project
julia> Pkg.activate("temp"; shared = true) # (edited)
┌ Error: `temp` is an invalid name for a shared environment.
│ To create a temporary environment use `Pkg.activate(; temp = true)`
└ @ Main REPL[10]:2

🚀 If instead Julia should check for a shared-env-temp, I'll modify load_path_expand as the snippet below:

the alternate
function load_path_expand(env::AbstractString)::Union{String, Nothing}
    # named environment?
    if startswith(env, '@')
        # `@.` in JULIA_LOAD_PATH is expanded early (at startup time)
        # if you put a `@.` in LOAD_PATH manually, it's expanded late
        env == "@" && return active_project(false)
        env == "@." && return current_project()
        env == "@stdlib" && return Sys.STDLIB
        if startswith(env, "@scriptdir")
            if @isdefined(PROGRAM_FILE)
                dir = dirname(PROGRAM_FILE)
            else
                cmds = unsafe_load_commands(opts.commands)
                if any((cmd, arg)->cmd_suppresses_program(cmd), cmds)
                    # Usage error. The user did not pass a script.
                    return nothing
                end
                dir = dirname(ARGS[1])
            end
            return abspath(replace(env, "@scriptdir" => dir))
        end
        env = replace(env, '#' => VERSION.major, count=1)
        env = replace(env, '#' => VERSION.minor, count=1)
        env = replace(env, '#' => VERSION.patch, count=1)
        name = env[2:end]
        temp = name == "temp" ? true : false
        # look for named env in each depot
        for depot in DEPOT_PATH
            path = joinpath(depot, "environments", name)
            isdir(path) || continue
            for proj in project_names
                file = abspath(path, proj)
                if isfile_casesensitive(file)
                    temp && @warn """
                    Activiating the pre-existing shared environment named `temp` at
                    $file."""
                    return file
                end
            end
        end
        isempty(DEPOT_PATH) && return nothing
        temp && (return mktempdir()) || (return abspath(DEPOT_PATH[1], "environments", name, project_names[end]))
    end
    # otherwise, it's a path
    path = abspath(env)
    if isdir(path)
        # directory with a project file?
        for proj in project_names
            file = joinpath(path, proj)
            isfile_casesensitive(file) && return file
        end
    end
    # package dir or path to project file
    return path
end

Please indicate with 👍 or 🚀.

@ven-k ven-k changed the title Set temporary env whenever Julia is started with --project=--temp Set temporary env whenever Julia is started with --project=@temp Feb 13, 2024
test/loading.jl Outdated
@@ -683,7 +683,7 @@ end

# Base.active_project when version directory exist in depot, but contains no project file
mktempdir() do dir
vdir = Base.DEFAULT_LOAD_PATH[2]
vdir = Base.DEFAULT_LOAD_PATH[3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change?

@mkitti
Copy link
Contributor

mkitti commented Feb 13, 2024

julia> Pkg.activate(; shared = "temp")
┌ Error: `temp` is an invalid name for a shared environment.
│ To create a temporary environment use `Pkg.activate(; temp = true)`
└ @ Main REPL[10]:2

I thought the shared keyword only accepted a Bool?

Also, we really should check for an existing shared environment called temp.

base/initdefs.jl Outdated
@@ -131,21 +132,23 @@ end
# this will inherit an existing JULIA_LOAD_PATH value or if there is none, leave
# a trailing empty entry in JULIA_LOAD_PATH which will be replaced with defaults.

const DEFAULT_LOAD_PATH = ["@", "@v#.#", "@stdlib"]
const DEFAULT_LOAD_PATH = ["@", "@temp", "@v#.#", "@stdlib"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. I thought @StefanKarpinski was saying that @oxinabox could set this in her startup.jl, but this is changing the default for everyone.

We do not want to change the default. That would be a breaking change.

@KristofferC
Copy link
Member

#49061 FTW!

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Feb 15, 2024

Modulo @mkitti's comment about not changing the default LOAD_PATH, this seems great and triage approves. 💟

@ven-k
Copy link
Member Author

ven-k commented Feb 15, 2024

base/initdefs.jl Outdated
Comment on lines 241 to 242
startswith(project, "@") ? load_path_expand(project) :
abspath(expanduser(project))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a formatting change? From a formatting perspective, I think indentation of line 242 would be useful.

From a soruce control perspective, I would prefer no change since this does not seem necessary for the feature being added
.

Suggested change
startswith(project, "@") ? load_path_expand(project) :
abspath(expanduser(project))
startswith(project, "@") ? load_path_expand(project) :
abspath(expanduser(project))

Copy link
Member Author

Choose a reason for hiding this comment

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

Indenting just the line 242 wouldn't do, considering previous 2 lines are also the else-options for ?:
But I agree, this change isn't necessary. I've reverted that here.

@mkitti
Copy link
Contributor

mkitti commented Feb 15, 2024

Thank you for the changes. I think this is mergeable.

I remain concerned about the unusual case where there does exist a shared environment called "temp" that people may have accessed via @temp in the past. I could have easily created such an environment when debugging or testing some set of packages.

We should consider checking if such an environment exists and issuing a warning in that case. That said, it could be another pull request.

@JeffBezanson
Copy link
Member

Let's rebase and merge this.

@ven-k ven-k force-pushed the vkb/temp-project branch from b203b09 to 684a34f Compare June 1, 2024 19:17
ven-k added 2 commits June 4, 2024 17:01
- Update relevant docs.
- Add tests for `--project=@temp` and `JULIA_PROJECT="@temp"`.

This makes it convenient to start and run Julia in a temporary environment. This brings `Pkg.activate(; temp=true)` functionality to cmdline.
@ven-k ven-k force-pushed the vkb/temp-project branch from 684a34f to 665889e Compare June 4, 2024 11:31
@oscardssmith oscardssmith added packages Package management and loading merge me PR is reviewed. Merge when all tests are passing and removed forget me not PRs that one wants to make sure aren't forgotten labels Jun 4, 2024
@DilumAluthge DilumAluthge merged commit 583981f into JuliaLang:master Jun 5, 2024
10 checks passed
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 5, 2024
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.