wip: Pkg3-style package loading #25333

StefanKarpinski · 2017-12-30T18:27:47Z

This works for julia --compiled-modules=no but fails in the presence of module precompilation, e.g. with the following output:

julia> import SHA
77394: find_package(Main, SHA)
77394: find_package_in_project(/Users/stefan/projects/Pkg3.jl/Project.toml, SHA)
77394: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, ea8e919c-243c-51af-8825-aaa63cd721ce, IOStream(<file /Users/stefan/projects/Pkg3.jl/Manifest.toml>))
77394: find_package(Main, SHA)
77394: find_package_in_project(/Users/stefan/projects/Pkg3.jl/Project.toml, SHA)
77394: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, ea8e919c-243c-51af-8825-aaa63cd721ce, IOStream(<file /Users/stefan/projects/Pkg3.jl/Manifest.toml>))
[ Info: Precompiling module SHA @ Base loading.jl:960
77395: find_package(SHA, Compat)
77395: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, ea8e919c-243c-51af-8825-aaa63cd721ce, Compat)
77395: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, Compat, IOStream(<file /Users/stefan/projects/Pkg3.jl/Manifest.toml>))
77395: find_package(SHA, Compat)
77395: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, ea8e919c-243c-51af-8825-aaa63cd721ce, Compat)
77395: find_package_in_manifest(/Users/stefan/projects/Pkg3.jl/Manifest.toml, Compat, IOStream(<file /Users/stefan/projects/Pkg3.jl/Manifest.toml>))
77394: find_package(Main, Compat)
77394: find_package_in_project(/Users/stefan/projects/Pkg3.jl/Project.toml, Compat)
┌ Warning: Module `SHA` declares `__precompile__(true)` but `require` failed
│ to create a usable precompiled cache file
│   exception = ErrorException("Required dependency Compat not found in current path.")
└ @ Base loading.jl:774
ERROR: Module SHA declares __precompile__(true) but require failed to create a usable precompiled cache file.
Stacktrace:
 [1] error at ./error.jl:33 [inlined]
 [2] _require(::Module, ::Symbol) at ./loading.jl:777
 [3] require(::Module, ::Symbol) at ./loading.jl:626

This is from the Pkg3 package since it's one of the only examples at this point of a project with Pkg3-style project and manifest files. @vtjnash: I think the ball is in your court here – I've tried to get this to work with module precompilation, but the logic of that code is beyond me. The fundamental problem seems to be that while Compat is loaded into SHA in the precompile child process and in the main process in the absence of module precompilation, when precompilation is on, the main process tries to load Compat into Main instead of SHA, which is incorrect and leads to a load failure.

The other major todo is to change the root module key to being a UUID instead of a name when a module is resolved by UUID, but that can be done in a separate PR since it's strictly internal and non-breaking (it adds the ability to have multiple root modules loaded with the same name).

StefanKarpinski · 2017-12-30T19:08:39Z

base/loading.jl

    end
 end
+SHA1(s::Union{String,SubString{String}}) = SHA1(hex2bytes(s))


This is just internal, we should not export this type or anything and should maybe replace it with something else, but for now it can live here and we can clean this stuff up later.

StefanKarpinski · 2017-12-30T19:11:25Z

base/initdefs.jl

    vers = "v$(VERSION.major).$(VERSION.minor)"
-    if haskey(ENV, "JULIA_LOAD_PATH")
-        prepend!(LOAD_PATH, split(ENV["JULIA_LOAD_PATH"], @static Sys.iswindows() ? ';' : ':'))
-    end
    push!(LOAD_PATH, abspath(BINDIR, "..", "local", "share", "julia", "site", vers))
    push!(LOAD_PATH, abspath(BINDIR, "..", "share", "julia", "site", vers))


This currently leaves the site paths in here and the second one is where stdlib lives, which we need. However, I think that we should probably have a Project.toml and Manifest.toml for Julia itself which includes these. The installed stdlib packages should probably be in a builtin/standard depot instead, but that's kind of a detail. We can always put relative paths into stdlib from the manifest file.

StefanKarpinski · 2017-12-30T19:13:07Z

base/loading.jl

+        if info isa Tuple{String,String,UUID}
+            # horrible hack, but since we don't control module creation, ¯\_(ツ)_/¯
+            ccall(:jl_set_global, Cvoid, (Any, Any, Any), __toplevel__, ENVINFO, info[2:end])
+        end


If we constructed the module object to be loaded and then evaluated code into it, then we could explicitly set the ENVINFO name in there instead of doing nasty stuff like this. I think we should probably do that, but I didn't want to couple this change with a change like that, so I just hacked around it instead.

StefanKarpinski · 2017-12-30T19:14:16Z

base/loading.jl

@@ -471,26 +804,23 @@ include_string(m::Module, txt::String, fname::String) =
 include_string(m::Module, txt::AbstractString, fname::AbstractString="string") =
    include_string(m, String(txt), String(fname))

-function source_path(default::Union{AbstractString,Nothing}="")
+function tls_recurse(key::Symbol, default=nothing)


I was at some point trying passing ENVINFO via task local storage, which is why I refactored this, but I didn't end up using it. I'm leaving the refactoring in since it's good to separate the TLS recursion from source path lookup anyway.

This method is deprecated (v0.7) anyways, I'm not sure it matters

StefanKarpinski · 2017-12-30T19:15:27Z

base/loading.jl

+                  const $ENVINFO = $envinfo
+                  # same horrible hack as in _require
+                  ccall(:jl_set_global, Cvoid, (Any, Any, Any),
+                        Base.__toplevel__, $(Meta.quot(ENVINFO)), $envinfo)


This allows correct new-style module loading in the child precompile process. The failure is later, however, when the precompiled module needs to get loaded into the original parent process, which tries to load it into Main.

StefanKarpinski · 2017-12-30T19:16:28Z

base/reflection.jl

+
+Get the root module enclosing `m`: this is the top-level module loaded by
+`import` or `using` which ultimately create `m`. This root module is found by
+iteratively taking a module's parent until self-parented module is found.


I didn't mean to leave this in the PR – it was something I wrote and then didn't end up using, but it's somewhat useful, so maybe we should leave it in.

StefanKarpinski · 2017-12-30T19:18:05Z

src/dump.c

@@ -1927,7 +1927,7 @@ static int size_isgreater(const void *a, const void *b)
    return *(size_t*)b - *(size_t*)a;
 }

-static jl_value_t *read_verify_mod_list(ios_t *s, arraylist_t *dependent_worlds, jl_array_t *mod_list)
+static jl_value_t *read_verify_mod_list(jl_module_t *from, ios_t *s, arraylist_t *dependent_worlds, jl_array_t *mod_list)


Oops: should this be called from or into? I'm not sure what this module is for. @JeffBezanson, @vtjnash?

I think this is into since it should verify the module list with respect to the environment the module is loaded into.

This isn't loading a module into an environment, it's just parsing a file and asserting that the header looks valid. @StefanKarpinski You should drop all changes to this file; my earlier PR to loading.jl moved everything into Julia that matters for Pkg3.

vchuravy · 2017-12-31T11:03:02Z

base/initdefs.jl

 function init_load_path(BINDIR = Sys.BINDIR)
+    load_path = get(ENV, "JULIA_LOAD_PATH", "@|@v#.#.#|@v#.#|@v#|@default|@!v#.#")


For other readers see JuliaLang/Pkg.jl#82 for context.
The string "@|@v#.#.#|@v#.#|@v#|@default|@!v#.#" turns into:

LOAD_PATH = Any[ [ CurrentEnv(), NamedEnv(“v$(VERSION.major).$(VERSION.minor).$(VERSION.patch)”), NamedEnv(“v$(VERSION.major).$(VERSION.minor)”), NamedEnv(“v$(VERSION.major)”), NamedEnv(“default”), NamedEnv(“v$(VERSION.major).$(VERSION.minor)”, create=true) ] ]

Yes, we need to bikeshed this syntax, but this is what I ended up going with:

@ = current project environment found by looking in parent directories

@name = named environment if it already exists

@!name = named environment even if it doesn't already exist

# characters are replaced by the major, minor and patch version numbers in order

I decided not to introduce any escaping since you can always use .juliarc.jl to set the values programmatically. If you have an actual relative path name that starts with a @ you can always write it as ./@...

vchuravy

I am still trying to wrap my head around the implications of the environment. As far as I understand it is now possible to have the name of a package resolve to different packages/versions depending on the environment (and therefore) module the package is being loaded into. This leads me to think that we will need a cache-file per environment.

In general this looks good, even though it would benefit from comments. This is my third read and I am still not sure that I understand environments and their implications fully.

vchuravy · 2017-12-31T11:07:56Z

base/initdefs.jl

+        push!(empty!(DEPOT_PATH), map(expanduser, depots))
+    else
+        push!(DEPOT_PATH, joinpath(homedir(), ".julia"))
+    end


We should probably also add two paths under BINDIR similar to the two for LOAD_PATH

push!(LOAD_PATH, abspath(BINDIR, "..", "local", "share", "julia", "site", vers)) push!(LOAD_PATH, abspath(BINDIR, "..", "share", "julia", "site", vers))

vchuravy · 2017-12-31T11:22:25Z

base/loading.jl

+            end
+        elseif endswith(path, ".toml") # project file
+            info = find_package_in_project(path, name)
+            info != nothing && return info


info !== nothing

vchuravy · 2017-12-31T11:28:47Z

base/loading.jl

+        deps_str = uuid = nothing
+        for line in eachline(io)
+            if !in_deps
+                if ismatch(r"^\s*\[\s*\[\s*\w+\s*\]\s*\]\s*$", line)


These regexes are repeated for find_package_in_manifest(manifest_file, name, io).
I would suggest factor them out or refactoring the two methods.

vchuravy · 2017-12-31T11:37:25Z

base/loading.jl

+        for line in eachline(io)
+            # look for `$name = "$uuid"` entry
+            m = match(r"^(?:\s*(?:#.*)?|\s*(\w+)\s*=\s*\"(.*)\"\s*)$", line)
+            m == nothing && return nothing


m === nothing

vchuravy · 2017-12-31T11:56:39Z

base/sysimg.jl

-Base.require(:Distributed)
-Base.require(:Printf)
-Base.require(:Future)
+Base.require(Base, :Base64)


Should these be required into Base?

They are, aren't they?

yes (I'm assuming this does not bind the name Base.Base64, right?)

vchuravy · 2017-12-31T11:58:43Z

src/toplevel.c

@@ -186,6 +190,7 @@ jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex)
        newm->parent = parent_module;
        b->value = (jl_value_t*)newm;
        jl_gc_wb_binding(b, newm);
+        // copy parent environment into submodule


This comment seems detached or is this a TODO?

Ah, yes, that comment is a stray, left from a previous iteration where the above code was here.

vchuravy · 2017-12-31T12:18:40Z

src/dump.c

@@ -1927,7 +1927,7 @@ static int size_isgreater(const void *a, const void *b)
    return *(size_t*)b - *(size_t*)a;
 }

-static jl_value_t *read_verify_mod_list(ios_t *s, arraylist_t *dependent_worlds, jl_array_t *mod_list)
+static jl_value_t *read_verify_mod_list(jl_module_t *from, ios_t *s, arraylist_t *dependent_worlds, jl_array_t *mod_list)


I think this is into since it should verify the module list with respect to the environment the module is loaded into.

StefanKarpinski · 2017-12-31T19:19:06Z

I am still trying to wrap my head around the implications of the environment. As far as I understand it is now possible to have the name of a package resolve to different packages/versions depending on the environment (and therefore) module the package is being loaded into. This leads me to think that we will need a cache-file per environment.

In essence, in an explicit environment the project file assigns identities to top-level package names via a mapping from names to package UUIDs in the [deps] section of the project file. When loading indirect dependencies via a manifest file, names are mapped to UUIDs via individual [deps] section for each package that is loaded. That means that import A can load one package at the top level and a different thing while loading a dependency. This is necessary to make sure that dependencies keep working no matter what other name clashes may occur in the ecosystem.

Caching absolutely should happen on a per-environment level. However, only the first environment in LOAD_PATH is guaranteed to be loaded intact – the following environments may end up getting dependencies at versions that appear in earlier environments in LOAD_PATH. So all of the caching is really specific to the first environment in that sense.

vtjnash · 2017-12-31T19:57:51Z

base/loading.jl

+import Base.Random: UUID
+
+const SlugInt = UInt32 # max p = 4
+const chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"


tested and confirmed this is String(collect('A':'Z')) * String(collect('a':'z')) * String(collect('0':'9'))

which is 78 characters versus 64.

I agree that @vtjnash's way is more obviously free of errors.

I've already updated this to String(['A':'Z'; 'a':'z'; '0':'9']) locally, which is shorter and clearer than either.

vtjnash · 2017-12-31T20:00:29Z

base/loading.jl

-            modpath = find_package(string(modname))
-            modpath === nothing && return ErrorException("Required dependency $modname not found in current path.")
-            mod = _require_search_from_serialized(modname, String(modpath))
+            info = find_package(into, string(modname))


this (this thing that is being found) isn't being loaded into into, it's being loaded into mod

but also mod and modname aren't Module names, they're UUIDs

Except they're not actual UUIDs, they're the 64-bit integers you're calling "UUIDs". Also, why is modname called modname if it's not a module name? Can you throw me a bone here and tell me how to get the module associated with mod instead of just telling me what's wrong with the code?

The into argument to _require is what's passed in here and as far as I can tell, it results in the correct module being passed in here. I think you may not be understanding what I'm trying to accomplish here. I've tried to explain and we've not gotten anywhere; this all works correctly in the absence of module precompilation, so if you could please make a RP on top of this one that fixes all the precompilation stuff, that would be great.

As far as I can tell, mod is the name of the module that needs to be created. into is the module into which the binding for mod is to be created when the process is done. So it seems to me that mod is not the module that should be passed as the into argument here. Why do you think it is?

vtjnash · 2017-12-31T20:05:21Z

base/loading.jl

@@ -471,26 +804,23 @@ include_string(m::Module, txt::String, fname::String) =
 include_string(m::Module, txt::AbstractString, fname::AbstractString="string") =
    include_string(m, String(txt), String(fname))

-function source_path(default::Union{AbstractString,Nothing}="")
+function tls_recurse(key::Symbol, default=nothing)


This method is deprecated (v0.7) anyways, I'm not sure it matters

StefanKarpinski · 2018-01-02T14:35:55Z

Unrelated LAPACK failure and a timeout. The previous run was all green; this is just a rebase.

StefanKarpinski · 2018-01-03T14:55:49Z

I don't get how this passed any tests. It fails locally since loaded_modules is used but not defined anywhere.

vtjnash · 2018-01-03T15:33:16Z

None of them ran the tests (since it is not mergeable). Strange that they still post success.

This reverts commit f532093.

Thread an `into` module through all require-related calls so that we can look up the name of the module to be loaded in the context of the module into which it is being loaded. test/compile: comment out failing code

also only store the UUID of each module, not the manifest path

If `path` is `nothing` the package wasn't found If `uuid` is `nothing` the package has no UUID

StefanKarpinski · 2018-01-16T18:29:31Z

Superseded by #25455.

StefanKarpinski requested review from vtjnash, JeffBezanson and vchuravy December 30, 2017 18:27

StefanKarpinski assigned vtjnash Dec 30, 2017

StefanKarpinski added this to the 1.0 milestone Dec 30, 2017

StefanKarpinski commented Dec 30, 2017

View reviewed changes

StefanKarpinski force-pushed the sk/reqfrom branch from 74c9292 to 6a5ba9b Compare December 31, 2017 01:42

vchuravy reviewed Dec 31, 2017

View reviewed changes

StefanKarpinski force-pushed the sk/reqfrom branch from 6a5ba9b to d22587c Compare December 31, 2017 19:28

StefanKarpinski mentioned this pull request Dec 31, 2017

WIP: Pkg3-style package loading #24405

Closed

vtjnash reviewed Dec 31, 2017

View reviewed changes

StefanKarpinski force-pushed the sk/reqfrom branch from d22587c to df022d2 Compare January 2, 2018 00:38

StefanKarpinski and others added 7 commits January 3, 2018 13:24

Revert "require: refactor error handling"

e5a5c3e

This reverts commit f532093.

require(mod::Symbol) => require(into::Module, mod::Symbol)

c32eb19

Thread an `into` module through all require-related calls so that we can look up the name of the module to be loaded in the context of the module into which it is being loaded. test/compile: comment out failing code

new Pkg3-style code loading working without precompilation

17b68de

add Pkg.dir to LOAD_PATH (to keep Documenter working)

d1cbff8

loading: shorter more explicit definition of slug chars

7229391

track requires dependencies alongside include dependencies

86a08e4

toplevel.c: move stray comment

144f79b

StefanKarpinski force-pushed the sk/reqfrom branch from 61312e4 to 144f79b Compare January 3, 2018 18:25

StefanKarpinski added 4 commits January 7, 2018 23:31

loading: allow finding packages without actually loading code

38b6376

also only store the UUID of each module, not the manifest path

loading: add test project

c7f6b9e

loading: rename #ENVINFO to #uuid since that's all it is now

cfc0d50

find_package: always return (path, uuid) pair [ci skip]

6716675

If `path` is `nothing` the package wasn't found If `uuid` is `nothing` the package has no UUID

StefanKarpinski force-pushed the sk/reqfrom branch from ef99c2c to 6716675 Compare January 8, 2018 06:38

StefanKarpinski closed this Jan 16, 2018

DilumAluthge deleted the sk/reqfrom branch March 25, 2021 22:07

		function init_load_path(BINDIR = Sys.BINDIR)
		load_path = get(ENV, "JULIA_LOAD_PATH", "@\|@v#.#.#\|@v#.#\|@v#\|@default\|@!v#.#")

wip: Pkg3-style package loading #25333

wip: Pkg3-style package loading #25333

Conversation

StefanKarpinski commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy Dec 31, 2017 • edited by StefanKarpinski Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchuravy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Jan 2, 2018

StefanKarpinski commented Jan 3, 2018

vtjnash commented Jan 3, 2018

StefanKarpinski commented Jan 16, 2018

vchuravy Dec 31, 2017 •

edited by StefanKarpinski

Loading

vchuravy left a comment •

edited

Loading