-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Remove ArgumentError()
in parse_cache_header()
when @depot
cannot be resolved
#51989
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
staticfloat
force-pushed
the
sf/ignore_missing_srctext_caches
branch
from
November 2, 2023 04:27
888999d
to
c57dcee
Compare
…ot be resolved In the original implementation of relocatable package cache files, `parse_cache_header()` was made responsible for resolving `@depot/`-relative paths, and it threw an `ArgumentError()` if it could not find one. However, this could happen for a few reasons: 1. The `JULIA_DEPOT_PATH` could be set incorrectly, so the appropriate source text files could not be found in an appropriate `packages/` directory. 2. The package could have been upgraded, leaving a cache file for an older version on-disk, but no matching source files. For a concrete example of (2), see the following `bash` script: #!/bin/bash set -euo pipefail TEMP_PATH=$(mktemp -d) JULIA=${JULIA:-julia} echo JULIA: ${JULIA} export JULIA_DEPOT_PATH="${TEMP_PATH}" trap 'rm -rf ${TEMP_PATH}' EXIT # Create a `Foo.jl` that has an `internal.jl` within it FOO_DIR=${JULIA_DEPOT_PATH}/dev/Foo mkdir -p ${FOO_DIR}/src cat >${FOO_DIR}/Project.toml <<EOF name = "Foo" uuid = "00000000-0000-0000-0000-000000000001" version = "1.0.0" EOF cat >${FOO_DIR}/src/Foo.jl <<EOF module Foo include("internal.jl") end EOF cat >${FOO_DIR}/src/internal.jl <<EOF # Nothing to see here, folks EOF ${JULIA} -e "import Pkg; Pkg.develop(\"Foo\"); Pkg.precompile()" # Change `Foo` to no longer depend on `internal.jl`; this should invalidate its cache files cat >${FOO_DIR}/src/Foo.jl <<EOF module Foo end EOF rm -f ${FOO_DIR}/src/internal.jl # This should print `SUCCESS!`, not `FAILURE!` ${JULIA} -e 'using Foo; println("SUCCESS!")' || echo "FAILURE!" This pull request removes the `ArgumentError()`, as it seems reasonable to require `parse_cache_header()` to not throw errors in cases like these. We instead rely upon a higher level validation to reject a cachefile whose depot cannot be found, which should happen when `stale_cachefile()` later checks to ensure that all includes are found on-disk, (which will be false, as these paths start with `@depot/`, an unlikely path prefix to exist).
This prevents the unlikely but annoying scenario where the user has a `@depot/packages/Foo/QRSTV/src/Foo.jl` file in the current working directory.
staticfloat
force-pushed
the
sf/ignore_missing_srctext_caches
branch
from
November 2, 2023 06:07
c33e59e
to
d6857df
Compare
Added a test |
vchuravy
approved these changes
Nov 2, 2023
vtjnash
approved these changes
Nov 2, 2023
fatteneder
reviewed
Nov 2, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
And I think this solves the problem of silent PkgEval skips which I discovered yesterday in #51746 (comment).
[ But it does not address the real problem there which IIUC is the use of dep/build.jl
in the pkgs. ]
fatteneder
added a commit
to fatteneder/julia
that referenced
this pull request
Jan 4, 2024
fatteneder
added a commit
to fatteneder/julia
that referenced
this pull request
Jan 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the original implementation of relocatable package cache files,
parse_cache_header()
was made responsible for resolving@depot/
-relative paths, and it threw anArgumentError()
if it could not find one. However, this could happen for a few reasons:The
JULIA_DEPOT_PATH
could be set incorrectly, so the appropriate source text files could not be found in an appropriatepackages/
directory.The package could have been upgraded, leaving a cache file for an older version on-disk, but no matching source files.
For a concrete example of (2), see the following
bash
script:This pull request removes the
ArgumentError()
, as it seems reasonable to requireparse_cache_header()
to not throw errors in cases like these. We instead rely upon a higher level validation to reject a cachefile whose depot cannot be found, which should happen whenstale_cachefile()
later checks to ensure that all includes are found on-disk, (which will be false, as these paths start with@depot/
, an unlikely path prefix to exist).