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

Remove ArgumentError() in parse_cache_header() when @depot cannot be resolved #51989

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

staticfloat
Copy link
Member

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).

@staticfloat staticfloat force-pushed the sf/ignore_missing_srctext_caches branch from 888999d to c57dcee Compare November 2, 2023 04:27
…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 staticfloat force-pushed the sf/ignore_missing_srctext_caches branch from c33e59e to d6857df Compare November 2, 2023 06:07
@staticfloat
Copy link
Member Author

Added a test

Copy link
Member

@fatteneder fatteneder left a 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. ]

test/loading.jl Show resolved Hide resolved
@staticfloat staticfloat merged commit 5eaad53 into master Nov 2, 2023
2 checks passed
@staticfloat staticfloat deleted the sf/ignore_missing_srctext_caches branch November 2, 2023 21:35
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants