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

Make Pkg more resilient to reading older manifests where ex-stdlibs are listed #3634

Merged
merged 3 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions src/Operations.jl
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,10 @@ tracking_registered_version(pkg::Union{PackageSpec, PackageEntry}, julia_version
!is_stdlib(pkg.uuid, julia_version) && pkg.path === nothing && pkg.repo.source === nothing

function source_path(manifest_file::String, pkg::Union{PackageSpec, PackageEntry}, julia_version = VERSION)
return is_stdlib(pkg.uuid, julia_version) ? Types.stdlib_path(pkg.name) :
pkg.path !== nothing ? joinpath(dirname(manifest_file), pkg.path) :
pkg.repo.source !== nothing ? find_installed(pkg.name, pkg.uuid, pkg.tree_hash) :
pkg.tree_hash !== nothing ? find_installed(pkg.name, pkg.uuid, pkg.tree_hash) :
nothing
pkg.tree_hash !== nothing ? find_installed(pkg.name, pkg.uuid, pkg.tree_hash) :
pkg.path !== nothing ? joinpath(dirname(manifest_file), pkg.path) :
is_any_stdlib(pkg.uuid, julia_version) ? Types.stdlib_path(pkg.name) :
nothing
end

#TODO rename
Expand Down Expand Up @@ -1068,12 +1067,9 @@ function build_versions(ctx::Context, uuids::Set{UUID}; verbose=false)
error("could not find entry with uuid $uuid in manifest $(ctx.env.manifest_file)")
end
name = entry.name
if entry.tree_hash !== nothing
path = find_installed(name, uuid, entry.tree_hash)
elseif entry.path !== nothing
path = project_rel_path(ctx.env, entry.path)
else
pkgerror("Could not find either `git-tree-sha1` or `path` for package $name")
path = source_path(ctx.env.manifest_file, entry)
if path === nothing
pkgerror("Failed to find path for package $name")
end
version = something(entry.version, v"0.0")
end
Expand Down
16 changes: 13 additions & 3 deletions src/Types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ using SHA
export UUID, SHA1, VersionRange, VersionSpec,
PackageSpec, PackageEntry, EnvCache, Context, GitRepo, Context!, Manifest, Project, err_rep,
PkgError, pkgerror, PkgPrecompileError,
has_name, has_uuid, is_stdlib, stdlib_version, is_unregistered_stdlib, stdlibs, write_env, write_env_usage, parse_toml,
has_name, has_uuid, is_stdlib, is_any_stdlib, stdlib_version, is_unregistered_stdlib, stdlibs, write_env, write_env_usage, parse_toml,
project_resolve!, project_deps_resolve!, manifest_resolve!, registry_resolve!, stdlib_resolve!, handle_repos_develop!, handle_repos_add!, ensure_resolved,
registered_name,
manifest_info,
Expand Down Expand Up @@ -430,20 +430,25 @@ is_project_uuid(env::EnvCache, uuid::UUID) = project_uuid(env) == uuid
# Context #
###########

const UPGRADABLE_STDLIBS = ["DelimitedFiles", "Statistics"]
const UPGRADABLE_STDLIBS_UUIDS = Set{UUID}()
const STDLIB = Ref{DictStdLibs}()
function load_stdlib()
stdlib = DictStdLibs()
for name in readdir(stdlib_dir())
# DelimitedFiles and Statistics are upgradable stdlibs
# TODO: Store this information of upgradable stdlibs somewhere else
name in ("DelimitedFiles", "Statistics") && continue
projfile = projectfile_path(stdlib_path(name); strict=true)
nothing === projfile && continue
project = parse_toml(projfile)
uuid = get(project, "uuid", nothing)::Union{String, Nothing}
v_str = get(project, "version", nothing)::Union{String, Nothing}
version = isnothing(v_str) ? nothing : VersionNumber(v_str)
nothing === uuid && continue
if name in UPGRADABLE_STDLIBS
push!(UPGRADABLE_STDLIBS_UUIDS, UUID(uuid))
continue
end
stdlib[UUID(uuid)] = (name, version)
end
return stdlib
Expand All @@ -455,7 +460,12 @@ function stdlibs()
end
return STDLIB[]
end
is_stdlib(uuid::UUID) = uuid in keys(stdlibs())
is_stdlib(uuid::UUID) = uuid in keys(stdlibs()) && uuid ∉ UPGRADABLE_STDLIBS_UUIDS
# Includes upgradable stdlibs
function is_any_stdlib(uuid::UUID, julia_version::Union{VersionNumber, Nothing})
return is_stdlib(uuid, julia_version) || uuid in UPGRADABLE_STDLIBS_UUIDS
end
Copy link
Member

@IanButterworth IanButterworth Sep 28, 2023

Choose a reason for hiding this comment

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

is_any_stdlib doesn't make sense to me.

I can't shake that it should be this, for correctness

Suggested change
is_stdlib(uuid::UUID) = uuid in keys(stdlibs()) && uuid UPGRADABLE_STDLIBS_UUIDS
# Includes upgradable stdlibs
function is_any_stdlib(uuid::UUID, julia_version::Union{VersionNumber, Nothing})
return is_stdlib(uuid, julia_version) || uuid in UPGRADABLE_STDLIBS_UUIDS
end
is_stdlib(uuid::UUID) = uuid in keys(stdlibs())
# Includes upgradable stdlibs
function is_upgradable_stdlib(uuid::UUID, julia_version::Union{VersionNumber, Nothing})
return uuid in UPGRADABLE_STDLIBS_UUIDS
end

And Pkg tests should check that UPGRADABLE_STDLIBS_UUIDS are all stdlibs probably

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed and tweaked some things slightly. I don't really think that test is needed since it is true "by construction" and the test would be written virtually identically to the implementation.



# Find the entry in `STDLIBS_BY_VERSION`
# that corresponds to the requested version, and use that.
Expand Down
9 changes: 9 additions & 0 deletions test/manifest/old/Manifest.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# This file is machine-generated - editing it directly is not advised

[[DelimitedFiles]]
deps = ["Mmap"]
uuid = "8bb1440f-4735-579b-a4ab-409b98df4dab"
version = "1.9.1"

[[Mmap]]
uuid = "a63ad114-7e13-5084-954f-fe012c677804"
2 changes: 2 additions & 0 deletions test/manifest/old/Project.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[deps]
DelimitedFiles = "8bb1440f-4735-579b-a4ab-409b98df4dab"
9 changes: 9 additions & 0 deletions test/new.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1456,6 +1456,15 @@ end
end
end
end
# instantiate old manifest
isolate(loaded_depot=true) do
manifest_dir = joinpath(@__DIR__, "manifest", "old")
cd(manifest_dir) do
Pkg.activate(".")
Pkg.instantiate()
@test isinstalled("DelimitedFiles")
end
end
# `instantiate` on a lonely manifest should detect duplicate names
isolate(loaded_depot=true) do; mktempdir() do tempdir
simple_package_path = copy_test_package(tempdir, "SimplePackage")
Expand Down