RFC: when loading code for internal purposes, load stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks #53326
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
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
vtjnash
force-pushed
the
jn/reliable-stdlibs
branch
3 times, most recently
from
a7ee8c9
to
2f2b2f3
Compare
…TH, LOAD_PATH, and stale checks Helps avoid the situation that the user can break important Pkg, REPL, Socket or similar features simply by choosing to remove @StdLib from their environment.
Preferable for all precompile code to finish loading, and not interrupt it to keep running various callbacks.
vtjnash
force-pushed
the
jn/reliable-stdlibs
branch
from
2f2b2f3
to
4f58564
Compare
KristofferC
pushed a commit
that referenced
this pull request
Feb 26, 2024
…53326) This bypasses DEPOT_PATH, LOAD_PATH, and stale checks when loading known stdlib code for known purposes from known locations, specifically to avoid the problem that I cannot fix my tools because I have used my tools to break my tools. This helps avoid the situation that the user can break important Pkg, REPL, Socket or similar features simply because they chose to remove `@stdlib` from their environment. For example, if you make an edit to REPL, then this will trigger recompilation and load the edited version: ``` $ ./julia -e 'using REPL' -iq ┌ Info: Precompiling REPL [3fa0cd96-eef1-5676-8a61-b3b8758bbffb] (cache misses: include_dependency fsize change (2), invalid header (10), mismatched flags (1)) └ @ Base loading.jl:2643 julia> ``` But this will load the version that shipped with Julia, regardless of the state of the cache or the source code (unless you delete the cache files): ``` $ ./julia -iq julia> ``` Fixes #53365 (cherry picked from commit ea1a0d2)
vtjnash
added a commit
that referenced
this pull request
Feb 29, 2024
Noticed in CI that `Base.runtests(["fail"])` fails here instead of throwing the correct error later, since #53326.
vtjnash
added a commit
that referenced
this pull request
Mar 1, 2024
Noticed in CI that `Base.runtests(["fail"])` fails here instead of throwing the correct error later, since #53326.
KristofferC
added a commit
that referenced
this pull request
Mar 1, 2024
Backported PRs: - [x] #53361 <!-- 🤖 [master] Bump the SparseArrays stdlib from c9f7293 to cb602d7 --> - [x] #53300 <!-- allow external absint to hold custom data in `codeinst.inferred` --> - [x] #53342 <!-- Add `Base.wrap` to docs --> - [x] #53372 <!-- Silence warnings in `test/file.jl` --> - [x] #53357 <!-- 🤖 [master] Bump the Pkg stdlib from 6dd0e7c9e to 76070d295 --> - [x] #53373 <!-- fix sysimage-native-code=no option with pkgimages --> - [x] #53333 <!-- More consistent return value for annotations, take 2 --> - [x] #53354 <!-- fix code coverage bug in tail position and `else` --> - [x] #53407 <!-- fix sysimage-native-code=yes option --> - [x] #53388 <!-- Fix documentation: thread pool of main thread --> - [x] #53355 <!-- Fix synchronization issues on the GC scheduler. --> - [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if inputs contain no ∃ typevar. --> - [x] #53437 <!-- Add debug variant of loader_trampolines.o --> - [x] #53284 <!-- Add annotate! method for AnnotatedIOBuffer --> - [x] #53466 <!-- [MozillaCACerts_jll] Update to v2023-12-12 --> - [x] #53467 <!-- [LibGit2_jll] Update to v1.7.2 --> - [x] #53326 <!-- RFC: when loading code for internal purposes, load stdlib files directly, bypassing DEPOT_PATH, LOAD_PATH, and stale checks --> - [x] #53332 - [x] #53320 <!-- Add `Sys.isreadable, Sys.iswriteable`, update `ispath` --> - [x] #53476 Contains multiple commits, manual intervention needed: - [ ] #53285 <!-- Add update mechanism for Terminfo, and common user-alias data --> Non-merged PRs with backport label: - [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing queue --> - [ ] #53408 <!-- task splitting: change additive accumulation to multiplicative --> - [ ] #53403 <!-- Move parallel precompilation to Base --> - [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [ ] #53391 <!-- Default to the medium code model in x86 linux --> - [ ] #53125 <!-- coverage: count coverage where explicitly requested by inference only --> - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility -->
KristofferC
pushed a commit
that referenced
this pull request
Mar 1, 2024
tecosaur
pushed a commit
to tecosaur/julia
that referenced
this pull request
Mar 4, 2024
…uliaLang#53326) This bypasses DEPOT_PATH, LOAD_PATH, and stale checks when loading known stdlib code for known purposes from known locations, specifically to avoid the problem that I cannot fix my tools because I have used my tools to break my tools. This helps avoid the situation that the user can break important Pkg, REPL, Socket or similar features simply because they chose to remove `@stdlib` from their environment. For example, if you make an edit to REPL, then this will trigger recompilation and load the edited version: ``` $ ./julia -e 'using REPL' -iq ┌ Info: Precompiling REPL [3fa0cd96-eef1-5676-8a61-b3b8758bbffb] (cache misses: include_dependency fsize change (2), invalid header (10), mismatched flags (1)) └ @ Base loading.jl:2643 julia> ``` But this will load the version that shipped with Julia, regardless of the state of the cache or the source code (unless you delete the cache files): ``` $ ./julia -iq julia> ``` Fixes JuliaLang#53365
tecosaur
pushed a commit
to tecosaur/julia
that referenced
this pull request
Mar 4, 2024
Noticed in CI that `Base.runtests(["fail"])` fails here instead of throwing the correct error later, since JuliaLang#53326.
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Mar 7, 2024
…uliaLang#53326) This bypasses DEPOT_PATH, LOAD_PATH, and stale checks when loading known stdlib code for known purposes from known locations, specifically to avoid the problem that I cannot fix my tools because I have used my tools to break my tools. This helps avoid the situation that the user can break important Pkg, REPL, Socket or similar features simply because they chose to remove `@stdlib` from their environment. For example, if you make an edit to REPL, then this will trigger recompilation and load the edited version: ``` $ ./julia -e 'using REPL' -iq ┌ Info: Precompiling REPL [3fa0cd96-eef1-5676-8a61-b3b8758bbffb] (cache misses: include_dependency fsize change (2), invalid header (10), mismatched flags (1)) └ @ Base loading.jl:2643 julia> ``` But this will load the version that shipped with Julia, regardless of the state of the cache or the source code (unless you delete the cache files): ``` $ ./julia -iq julia> ``` Fixes JuliaLang#53365
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Apr 13, 2024
Noticed in CI that `Base.runtests(["fail"])` fails here instead of throwing the correct error later, since JuliaLang#53326.
This was referenced May 24, 2024
fingolfin
pushed a commit
that referenced
this pull request
May 24, 2024
TL;DR: we changed to loading stdlibs exclusively from the default sysimg in #53326, and this breaks developing Distributed.jl because the workers will load the builtin version instead of the development version. I think this should be backported to 1.11? CC @jpsamaroo --- `Base.require_stdlib()` will exclusively load a stdlib from whatever was shipped with Julia. The problem is that when developing Distributed.jl this will cause the workers to always load the builtin Distributed module instead of the development version, which can break everything because now the master and worker are running different (potentially incompatible) versions of Distributed. This commit changes Base and LinearAlgebra to use `Base.require()` instead, which will respect `Base.LOAD_PATH`. I argue that this is safe because unlike the other stdlibs like REPL or Pkg, Distributed is only loaded if explicitly requested by the user with `-p` or through `addprocs()` or something, so it shouldn't be possible to get into quite the same tools-are-broken-because-I-broke-my-tools situation that motivated using `Base.require_stdlib()` in the first place. An alternative design would be to: 1. Move the if-block loading Distributed in `exec_options()` below the for-loop where it will execute `-e` options. 2. Require any implementation of `Distributed.launch(::ClusterManager)` to pass `-e 'using Distributed'` in their command to ensure that Distributed is loaded in a way respecting `Base.LOAD_PATH`. This would be more consistent with how the other stdlibs must be developed, but it requires implementers (i.e. Distributed and ClusterManagers.jl) to opt-in to allowing development versions of Distributed, which feels very annoying and easy to miss so I decided not to implement that.
KristofferC
pushed a commit
that referenced
this pull request
May 27, 2024
TL;DR: we changed to loading stdlibs exclusively from the default sysimg in #53326, and this breaks developing Distributed.jl because the workers will load the builtin version instead of the development version. I think this should be backported to 1.11? CC @jpsamaroo --- `Base.require_stdlib()` will exclusively load a stdlib from whatever was shipped with Julia. The problem is that when developing Distributed.jl this will cause the workers to always load the builtin Distributed module instead of the development version, which can break everything because now the master and worker are running different (potentially incompatible) versions of Distributed. This commit changes Base and LinearAlgebra to use `Base.require()` instead, which will respect `Base.LOAD_PATH`. I argue that this is safe because unlike the other stdlibs like REPL or Pkg, Distributed is only loaded if explicitly requested by the user with `-p` or through `addprocs()` or something, so it shouldn't be possible to get into quite the same tools-are-broken-because-I-broke-my-tools situation that motivated using `Base.require_stdlib()` in the first place. An alternative design would be to: 1. Move the if-block loading Distributed in `exec_options()` below the for-loop where it will execute `-e` options. 2. Require any implementation of `Distributed.launch(::ClusterManager)` to pass `-e 'using Distributed'` in their command to ensure that Distributed is loaded in a way respecting `Base.LOAD_PATH`. This would be more consistent with how the other stdlibs must be developed, but it requires implementers (i.e. Distributed and ClusterManagers.jl) to opt-in to allowing development versions of Distributed, which feels very annoying and easy to miss so I decided not to implement that. (cherry picked from commit 3b922b0)
DilumAluthge
pushed a commit
that referenced
this pull request
Jun 3, 2024
TL;DR: we changed to loading stdlibs exclusively from the default sysimg in #53326, and this breaks developing Distributed.jl because the workers will load the builtin version instead of the development version. I think this should be backported to 1.11? CC @jpsamaroo --- `Base.require_stdlib()` will exclusively load a stdlib from whatever was shipped with Julia. The problem is that when developing Distributed.jl this will cause the workers to always load the builtin Distributed module instead of the development version, which can break everything because now the master and worker are running different (potentially incompatible) versions of Distributed. This commit changes Base and LinearAlgebra to use `Base.require()` instead, which will respect `Base.LOAD_PATH`. I argue that this is safe because unlike the other stdlibs like REPL or Pkg, Distributed is only loaded if explicitly requested by the user with `-p` or through `addprocs()` or something, so it shouldn't be possible to get into quite the same tools-are-broken-because-I-broke-my-tools situation that motivated using `Base.require_stdlib()` in the first place. An alternative design would be to: 1. Move the if-block loading Distributed in `exec_options()` below the for-loop where it will execute `-e` options. 2. Require any implementation of `Distributed.launch(::ClusterManager)` to pass `-e 'using Distributed'` in their command to ensure that Distributed is loaded in a way respecting `Base.LOAD_PATH`. This would be more consistent with how the other stdlibs must be developed, but it requires implementers (i.e. Distributed and ClusterManagers.jl) to opt-in to allowing development versions of Distributed, which feels very annoying and easy to miss so I decided not to implement that.
lazarusA
pushed a commit
to lazarusA/julia
that referenced
this pull request
Jul 12, 2024
TL;DR: we changed to loading stdlibs exclusively from the default sysimg in JuliaLang#53326, and this breaks developing Distributed.jl because the workers will load the builtin version instead of the development version. I think this should be backported to 1.11? CC @jpsamaroo --- `Base.require_stdlib()` will exclusively load a stdlib from whatever was shipped with Julia. The problem is that when developing Distributed.jl this will cause the workers to always load the builtin Distributed module instead of the development version, which can break everything because now the master and worker are running different (potentially incompatible) versions of Distributed. This commit changes Base and LinearAlgebra to use `Base.require()` instead, which will respect `Base.LOAD_PATH`. I argue that this is safe because unlike the other stdlibs like REPL or Pkg, Distributed is only loaded if explicitly requested by the user with `-p` or through `addprocs()` or something, so it shouldn't be possible to get into quite the same tools-are-broken-because-I-broke-my-tools situation that motivated using `Base.require_stdlib()` in the first place. An alternative design would be to: 1. Move the if-block loading Distributed in `exec_options()` below the for-loop where it will execute `-e` options. 2. Require any implementation of `Distributed.launch(::ClusterManager)` to pass `-e 'using Distributed'` in their command to ensure that Distributed is loaded in a way respecting `Base.LOAD_PATH`. This would be more consistent with how the other stdlibs must be developed, but it requires implementers (i.e. Distributed and ClusterManagers.jl) to opt-in to allowing development versions of Distributed, which feels very annoying and easy to miss so I decided not to implement that.
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.
This helps avoid the situation that the user can break important Pkg, REPL, Socket or similar features simply because they chose to remove
@stdlibfrom their environment. For example, if you make an edit to REPL, then this will trigger recompilation and load the edited version:But this will load the version that shipped with Julia, regardless of the state of the cache or the source code (unless you delete the cache files):
Fixes #53365