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

Fix julia#55850 by using safe_realpath instead of abspath in projname #4025

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

christiangnrd
Copy link
Contributor

@christiangnrd christiangnrd commented Sep 24, 2024

Replaces REPLExt.projname with REPL.projname since they are identical.

Will solve JuliaLang/julia#55850 for situations where Pkg is loaded once JuliaLang/julia#55851 is merged.

Test will be covered by JuliaLang/julia#55851.

@christiangnrd christiangnrd changed the title Fix #55850 by using safe_realpath insterad of abspath Use projname from REPL.jl Sep 24, 2024
@christiangnrd
Copy link
Contributor Author

@IanButterworth This is also ready for review.

@IanButterworth
Copy link
Member

Just to highlight because the diff isn't clear in either PR, the difference between the two projnames is that Pkg uses
Types.read_project to get the project name, where REPL's directly uses Base.TOML.Parser.

I don't see an issue switching over because there's no side effects/caching of the project in either.

Pkg's

function projname(project_file::String)
    project = try
        Types.read_project(project_file)
    catch
        nothing
    end
    if project === nothing || project.name === nothing
        name = basename(dirname(project_file))
    else
        name = project.name::String
    end
    for depot in Base.DEPOT_PATH
        envdir = joinpath(depot, "environments")
        if startswith(abspath(project_file), abspath(envdir))
            return "@" * name
        end
    end
    return name
end

REPL's

function projname(project_file::String)
    if isfile(project_file)
        name = try
            # The `nothing` here means that this TOML parser does not return proper Dates.jl
            # objects - but that's OK since we're just checking the name here.
            p = Base.TOML.Parser{nothing}()
            Base.TOML.reinit!(p, read(project_file, String); filepath=project_file)
            proj = Base.TOML.parse(p)
            get(proj, "name", nothing)
        catch
            nothing
        end
    else
        name = nothing
    end
    if name === nothing
        name = basename(dirname(project_file))
    end
    for depot in Base.DEPOT_PATH
        envdir = joinpath(depot, "environments")
        if startswith(abspath(project_file), abspath(envdir))
            return "@" * name
        end
    end
    return name
end

IanButterworth
IanButterworth previously approved these changes Sep 24, 2024
@KristofferC
Copy link
Member

In my opinion I rather have some code duplication than start reaching into the internals of things. But that's just my opinion.

@IanButterworth
Copy link
Member

Ok, let's just implement the JuliaLang/julia#55851 fix in both then.

Sorry @christiangnrd for the flip flop

@christiangnrd christiangnrd changed the title Use projname from REPL.jl Fix julia#55850 by using safe_realpath instead of abspath in projname Sep 24, 2024
test/repl.jl Outdated Show resolved Hide resolved
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
@christiangnrd

This comment was marked as outdated.

test/repl.jl Outdated Show resolved Hide resolved
Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
@christiangnrd
Copy link
Contributor Author

This seems good to go on all platforms now.

IanButterworth pushed a commit that referenced this pull request Oct 5, 2024
…#4025)

* Update REPLExt.jl

* Add test

* Update test/repl.jl

Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>

* Update test/repl.jl

Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>

* Fix windows tests

---------

Co-authored-by: Ian Butterworth <i.r.butterworth@gmail.com>
(cherry picked from commit 2ad377f)
@IanButterworth IanButterworth mentioned this pull request Oct 5, 2024
7 tasks
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.

3 participants