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

load path: change default LOAD_PATH (#25709) #26804

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Conversation

StefanKarpinski
Copy link
Member

also:

  • rename "site" directory to "stdlib" since that's what it is now

  • use JULIA_LOAD_PATH as-is instead unconditionally appending the
    system package and stdlib directories to it

  • change default DEPOT_PATH to include system paths: one for
    arch-specific and one for arch-independent packages

  • delete comment about bundled code going in a versioned directory
    as it no longer applies since installed packages can and should
    be shared across different Julia versions

@StefanKarpinski StefanKarpinski added the packages Package management and loading label Apr 13, 2018
@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Apr 13, 2018
@@ -127,7 +127,6 @@ jl_symbol("pipe_writer"),
jl_symbol("idxfloor"),
jl_symbol("id"),
jl_symbol("ComplexF32"),
jl_symbol("/home/jeff/src/julia/usr/share/julia/site/v0.7/Pkg/src/resolve/versionweight.jl"),
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 do use this symbol all the time.

Copy link
Member

Choose a reason for hiding this comment

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

The data we have says that you do, in fact, use it all the time :)

`/bin/julia` will have a default `LOAD_PATH` of

```
/local/share/julia/site/v0.6
/share/julia/site/v0.6
/local/share/julia/stdlib/v0.7
Copy link
Member

Choose a reason for hiding this comment

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

If I read the code for init_load_path correctly /local/share is no longer used

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point.

@@ -13,8 +13,8 @@ function temp_pkg_dir(fn::Function)
pushfirst!(DEPOT_PATH, depot_dir)
# Add the standard library paths back
vers = "v$(VERSION.major).$(VERSION.minor)"
push!(LOAD_PATH, abspath(Sys.BINDIR, "..", "local", "share", "julia", "site", vers))
push!(LOAD_PATH, abspath(Sys.BINDIR, "..", "share", "julia", "site", vers))
push!(LOAD_PATH, abspath(Sys.BINDIR, "..", "local", "share", "julia", "stdlib", vers))
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

vers = "v$(VERSION.major).$(VERSION.minor)"
push!(LOAD_PATH, abspath(BINDIR, "..", "local", "share", "julia", "site", vers))
push!(LOAD_PATH, abspath(BINDIR, "..", "share", "julia", "site", vers))
stdlib = abspath(BINDIR, "..", "share", "julia", "stdlib", vers)
Copy link
Member

Choose a reason for hiding this comment

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

Is it kosher to have a directory environment nested inside a depot directory? I guess nothing prevents it, but it could maybe lead to confusion.

@@ -68,23 +68,13 @@ and a global configuration search path of
### `JULIA_LOAD_PATH`

A separated list of absolute paths that are to be appended to the variable
[`LOAD_PATH`](@ref). (In Unix-like systems, the path separator is `:`; in Windows
Copy link
Member

Choose a reason for hiding this comment

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

This should probably now say "used to set LOAD_PATH" instead of "appended to LOAD_PATH".

the absolute path
`$JULIA_HOME/../share/julia/stdlib/v$(VERSION.major).$(VERSION.minor)` so that,
e.g., version 0.7 of Julia on a Linux system with a Julia executable at
`/bin/julia` will have a default `LOAD_PATH` of `/share/julia/stdlib/v0.7`.
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to the doc string for LOAD_PATH.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of this really needs to be rewritten for the new code loading. I'm just trying to make the functional changes that are necessary here; the doc updates can be made in a separate pass.

also:

- rename "site" directory to "stdlib" since that's what it is now

- use JULIA_LOAD_PATH as-is instead unconditionally appending the
  system package and stdlib directories to it

- change default DEPOT_PATH to include system paths: one for
  arch-specific and one for arch-independent packages

- delete comment about bundled code going in a versioned directory
  as it no longer applies since installed packages can and should
  be shared across different Julia versions

- update Pkg3 and tests to work correctly when the stdlib directory
  isn't always included in LOAD_PATH

fix failing tests
@StefanKarpinski
Copy link
Member Author

AppVeyor timed out but got far enough to see that the previously-failing Pkg3 tests are now passing.

@StefanKarpinski StefanKarpinski merged commit 50413a4 into master Apr 19, 2018
@StefanKarpinski StefanKarpinski deleted the sk/load_path branch April 19, 2018 00:48
mbauman added a commit that referenced this pull request Apr 19, 2018
* origin/master: (22 commits)
  separate `isbitstype(::Type)` from `isbits` (#26850)
  bugfix for regex matches ending with non-ASCII (#26831)
  [NewOptimizer] track inbounds state as a per-statement flag
  change default LOAD_PATH and DEPOT_PATH (#26804, fix #25709)
  Change url scheme to https (#26835)
  [NewOptimizer] inlining: Refactor todo object
  inference: enable CodeInfo method_for_inference_limit_heuristics support (#26822)
  [NewOptimizer] Fix _apply elision (#26821)
  add test case from issue #26607, cfunction with no args (#26838)
  add `do` in front-end deparser. fixes #17781 (#26840)
  Preserve CallInst metadata in LateLowerGCFrame pass.
  Improve differences from R documentation (#26810)
  reserve syntax that could be used for computed field types (#18466) (#26816)
  Add support for Atomic{Bool} (Fix #26542). (#26597)
  Remove argument restriction on dims2string and inds2string (#26799) (#26817)
  remove some unnecessary `eltype` methods (#26791)
  optimize: ensure merge_value_ssa doesn't drop PiNodes
  inference: improve tmerge for Conditional and Const
  ensure more iterators stay type-stable
  code loading docs (#26787)
  ...
@maleadt
Copy link
Member

maleadt commented Apr 23, 2018

This PR and its docs seem to imply JULIA_LOAD_PATH should append to LOAD_PATH, but it looks like it's completely overriding instead? Testing on de705f3:

$ ./julia -e "display(LOAD_PATH)"
4-element Array{Any,1}:
 Base.CurrentEnv()                                                                                                                     
 Any[Base.NamedEnv("v0.7.0"), Base.NamedEnv("v0.7"), Base.NamedEnv("v0"), Base.NamedEnv("default"), Base.NamedEnv("v0.7", create=true)]
 "/home/tbesard/Julia/julia-dev/build/release/usr/share/julia/stdlib/v0.7"                                                             
 Pkg.dir

$ JULIA_LOAD_PATH=/tmp ./julia -e "display(LOAD_PATH)"
2-element Array{Any,1}:
 "/tmp" 
 Pkg.dir

This breaks, e.g., using Pkg after setting JULIA_LOAD_PATH in order to quickly pick up a package from a custom path. If that is unsupported now, what's an easy alternative?

EDIT: ah, just saw #26804 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants