rebase of #56 (implement clone/free) onto #81 (graph refactor) #83

carlobaldassi · 2017-12-17T23:11:39Z

As per the title, this rebases #56 on top of #81, cc @KristofferC, @StefanKarpinski.
I had to change a few things, and this allowed me to catch a few issues on the graph refactoring that I fixed. It seems to work now, or at least tests pass.

One annoying thing is that I had to bring back some pieces of Pkg2 from bin/ into src/, to allow for the parsing of REQUIRE files. I suppose that it's going to be needed as long as we support Pkg2-style dependency specifications ~~(that part of the code needs to be fixed BTW)~~.

carlobaldassi · 2017-12-18T10:46:45Z

Update: fixed the parsing of Pkg2 REQUIRE files.

StefanKarpinski · 2017-12-18T15:18:47Z

Thank you for doing this, @carlobaldassi!

carlobaldassi · 2017-12-18T18:59:40Z

I was feeling guilty for the amount of disruptiveness of my PRs :)

The Travis job fails with ERROR: LoadError: LoadError: ArgumentError: Module SHA not found in current path., while AppVeyor succeeds. Does somebody have a clue?

StefanKarpinski · 2017-12-18T19:02:53Z

Unfortunately, I'm not sure what's up with CI on this and @KristofferC is on vacation, so 🤷‍♂️

KristofferC · 2017-12-19T08:50:50Z

When I made this PR, SHA was not needed for Pkg3 itself, only for the bin stuff. This PR changes the .travis.yml script to only use Pkg3. We thus never do Pkg.clone and thus not download SHA.

carlobaldassi · 2017-12-21T23:42:36Z

Ok the issue with Travis testing is fixed now. It's rather crude, but then again this won't be a problem once Pkg3 is moved to stdlib I guess.

carlobaldassi · 2018-01-06T13:25:13Z

This is now on top of master, i.e. on top of all the graph refactoring stuff.

KristofferC · 2018-01-06T13:47:21Z

Lovely, thanks a lot!

andyferris · 2018-01-14T11:27:50Z

FYI I made a rebase of this at ajf/clonerepo2-rebased.

I found a problem where the tokenizer can't understand git urls like pkg> clone git@github.com:a/b (while the Pkg3.clone("git@github.com:a/b") works fine).

andyferris · 2018-01-14T11:53:57Z

Using this, this seems neat.

I'll note a couple things that would help me to use Pkg3 at work (eventually - doesn't have to be this PR):

Use something other than REQUIRE to get the dependencies (it can remain as a "backup option"). Apart from consistency with the rest of the .toml files, I would like to add private dependencies here without breaking Pkg2 compatibility with standard METADATA (this would help me getting a private registry running for Julia v0.6).
Allow one to specify a path inside the git repo as the base path of the package, since Pkg3 can support multiple packages per git repository using trees (this is going to be awesome, but probably not immediately necessary).

KristofferC · 2018-01-14T12:09:30Z

I found a problem where the tokenizer can't understand git urls..

For now, you need to surround the url with quotes.

Use something other than REQUIRE to get the dependencies (it can remain as a "backup option").

Definitely, but in order to make this PR right now (and to test with existing package) it needed to handle REQUIRE files.

since Pkg3 can support multiple packages per git repository using trees

It can, I didn't know this?

StefanKarpinski

Relatively minor comments, but overall this looks great. Sorry this took me so long to get around to reviewing.

StefanKarpinski · 2018-01-14T17:49:43Z

src/Display.jl

@@ -12,7 +12,7 @@ const colors = Dict(
    '↑' => :light_yellow,
    '~' => :light_yellow,
    '↓' => :light_magenta,
-    '?' => :red,
+    '?' => :white,


Seems unrelated?

There was some reason for this that made red look weird but I don't remember. Will change it back.

StefanKarpinski · 2018-01-14T17:56:23Z

src/Operations.jl

@@ -256,7 +303,7 @@ function version_data(env::EnvCache, pkgs::Vector{PackageSpec})
                end
            end
        end
-        @assert haskey(hashes, uuid)
+        # @assert haskey(hashes, uuid)


Is this no longer a valid assertion? If not we should remove this comment. If it's still valid, it should be uncommented.

StefanKarpinski · 2018-01-14T17:58:48Z

src/Operations.jl

@@ -351,7 +398,7 @@ function install(
    return version_path, true
 end

-function update_manifest(env::EnvCache, uuid::UUID, name::String, hash::SHA1, version::VersionNumber)
+function update_manifest(env::EnvCache, uuid::UUID, name::String, hash::Union{Void, SHA1}, version::VersionNumber, path::AbstractString)


Wouldn't it make more sense for the signature here to be Union{String, SHA1} and call it hash_or_path? Or is the deal here that path is always supplied but hash may not be? It seems like the code is using an empty path value to indicate lack of a path value but the either or pattern might be cleaner.

StefanKarpinski · 2018-01-14T18:04:49Z

src/Operations.jl

+            push!(pkgs, PackageSpec(r.package))
+        end
+        # TODO: Get rid of this one?
+        registry_resolve!(env, pkgs)


How do we determine if this should or shouldn't be gotten rid if?

The problem is using REQUIRE files that do not have UUIDs so the only way to get the UUIDS is to go to the registry. When we are not using REQUIRE files the UUIDs can just be read from the Manifest file.

This should be harmless – if all the packages are already resolved, it won't hit the registry at all.

StefanKarpinski · 2018-01-14T18:12:39Z

src/REPLMode.jl

+function do_clone!(env::EnvCache, tokens::Vector{Tuple{Symbol,Vararg{Any}}})
+    isempty(tokens) && cmderror("`clone` take an url to a package to clone")
+    local url
+    basepath = get(ENV, "JULIA_DEV_PATH", default_dev_path())


So even though this is a "path" to a place, we use _PATH and _PATH as a suffix in LOAD_PATH and DEPOT_PATH to indicate a list of places to look for things, whereas this is a single location to put things, which seems more like Sys.BINDIR or JULIA_PKGDIR so I think this environment variable should be called JULIA_DEVDIR or JULIA_DEV_DIR. I favor the former since we're going to have a bunch of variables with names like SRCDIR and LIBDIR so using the name DEVDIR seems more consistent.

StefanKarpinski · 2018-01-14T18:15:33Z

since Pkg3 can support multiple packages per git repository using trees

It can, I didn't know this?

It's not yet implemented, but since a version is just a tree and a tree doesn't need to appear at the root of a repository, it could with the right tooling work. Since we don't really have any dev tooling for tagging Pkg3 packages yet, it doesn't support this but it also doesn't not support this – if you arranged for a Pkg3 version entry to map to a subtree in the repo, it would just work. Of course that subtree should look like a normal package, but that's certainly doable.

andyferris · 2018-01-15T00:08:55Z

since a version is just a tree and a tree doesn't need to appear at the root of a repository, it could with the right tooling work. Since we don't really have any dev tooling for tagging Pkg3 packages yet, it doesn't support this but it also doesn't not support this – if you arranged for a Pkg3 version entry to map to a subtree in the repo, it would just work. Of course that subtree should look like a normal package, but that's certainly doable.

Exactly. AFAICT, if I make my own head-written registry, I can have my own monorepo for all of my company's private packages - that is implemented. (After reading some blogposts on Google's monorepo, it seems rather appealing, but in any case it would be good to put some related libraries and/or tools/apps/projects into one repository).

I would be extremely happy if any new tooling regarding creating and editing registries, package templates, and developing packages could respect this possibility - since adding this later might be difficult after architectural decisions are made and lead to avoidable refactoring.

Regarding the functionality in this PR, I guess packages in development mode need a local git repository (cloned in this case) and a subdirectory of the repository, which can then be mapped to-and-from a tree hash?

StefanKarpinski · 2018-01-15T04:27:36Z

My thinking is that if a MetaProject.toml file exists we look in that for where the actual projects are and dev tools all respect that.

andyferris · 2018-01-15T05:50:00Z

OK, cool, so this isn't recorded in the registry at all... actually that's quite nice and lets developers move stuff around inside the repository quite easily.

carlobaldassi · 2018-01-29T13:56:06Z

src/Operations.jl

+            !isfile(reqfile) && continue
+            for r in filter!(r->r isa Pkg2.Reqs.Requirement, Pkg2.Reqs.read(reqfile))
+                # @assert length(r.versions.intervals) == 1
+                pkg_name, version = r.package, r.versions.intervals[1]


I had fixed this (and the following lines) in bcdfe81

Sorry, must have messed up in rebasing :/

It would be preferable to apply the fixes to c728e4b, where the rebase went wrong

alright, I'll try...

Hopefully good now

Needs to bring back pieces of Pkg2 to parse old-style REQUIRE files

Work-around for bootstrapping problem in trying to use Pkg3 to clone itself

Needs to bring back pieces of Pkg2 to parse old-style REQUIRE files

Work-around for bootstrapping problem in trying to use Pkg3 to clone itself

KristofferC · 2018-03-12T08:25:50Z

Everything here should now be with similar functionality in master

carlobaldassi force-pushed the kc/clonerepo2-rebased branch 2 times, most recently from 4d9af18 to 5aaf9cf Compare December 18, 2017 10:10

carlobaldassi force-pushed the kc/clonerepo2-rebased branch from 3054a2a to 66e7d52 Compare December 18, 2017 11:00

carlobaldassi force-pushed the kc/clonerepo2-rebased branch from fb5142a to 24ac830 Compare December 21, 2017 23:30

carlobaldassi force-pushed the kc/clonerepo2-rebased branch from 24ac830 to 6d80313 Compare December 22, 2017 00:08

carlobaldassi mentioned this pull request Dec 22, 2017

Resolution log - looking into what the solver is doing, and why #86

Merged

carlobaldassi changed the base branch from master to cb/graphrefactor December 22, 2017 11:57

carlobaldassi mentioned this pull request Jan 5, 2018

Add backtracking to max-sum, improve greedy solver; the solver is now exact #94

Merged

carlobaldassi force-pushed the cb/graphrefactor branch from 22d0077 to 776439b Compare January 5, 2018 23:36

carlobaldassi force-pushed the kc/clonerepo2-rebased branch from 6d80313 to ee2bd89 Compare January 6, 2018 00:30

carlobaldassi changed the base branch from cb/graphrefactor to cb/backtrack January 6, 2018 00:31

carlobaldassi changed the base branch from cb/backtrack to master January 6, 2018 13:24

carlobaldassi mentioned this pull request Jan 6, 2018

implement cloning (and the inverse, freeing) #56

Closed

carlobaldassi force-pushed the kc/clonerepo2-rebased branch from ee2bd89 to 1f0f0ae Compare January 6, 2018 13:34

KristofferC closed this Jan 7, 2018

KristofferC reopened this Jan 7, 2018

KristofferC mentioned this pull request Jan 8, 2018

fixes for cloning (+ help requested) #100

Merged

StefanKarpinski reviewed Jan 14, 2018

View reviewed changes

add tokenization of strings

213ade8

KristofferC force-pushed the kc/clonerepo2-rebased branch from 1f0f0ae to d8c97b7 Compare January 29, 2018 08:44

carlobaldassi commented Jan 29, 2018

View reviewed changes

KristofferC and others added 5 commits January 29, 2018 15:03

implement clone and free

a984a81

Needs to bring back pieces of Pkg2 to parse old-style REQUIRE files

test a cloned unregistered package

aba4ab5

use a separate path_resolve! functions

6a27a15

Fix Pkg2-style REQUIRE parsing into Pkg3 types

8688c77

Explicitly Pkg.add("SHA") in Travis script

c80ef4d

Work-around for bootstrapping problem in trying to use Pkg3 to clone itself

KristofferC force-pushed the kc/clonerepo2-rebased branch from 9583edb to c80ef4d Compare January 29, 2018 14:06

KristofferC and others added 6 commits January 29, 2018 20:37

fixes for cloning

9551bf8

implement clone and free

5a1e7b3

Needs to bring back pieces of Pkg2 to parse old-style REQUIRE files

test a cloned unregistered package

1243828

use a separate path_resolve! functions

5a30b4e

Fix Pkg2-style REQUIRE parsing into Pkg3 types

eb05d94

Explicitly Pkg.add("SHA") in Travis script

cd4e43d

Work-around for bootstrapping problem in trying to use Pkg3 to clone itself

KristofferC force-pushed the kc/clonerepo2-rebased branch from c80ef4d to cd4e43d Compare January 29, 2018 20:22

KristofferC added 4 commits January 29, 2018 21:36

update based on review

44ed081

Merge branch 'kc/no_deps_or_reqs_for_fixed' into kc/clonerepo2-rebased

555ca7c

fixes to cloning and building cloned packages

6b62c36

only add version for packages with path

2805b01

KristofferC closed this Mar 12, 2018

KristofferC deleted the kc/clonerepo2-rebased branch March 15, 2018 10:28

songrunxian mentioned this pull request Sep 24, 2021

ERROR: could not download https://pkg.julialang.org/registry/23338594-aafe-5451-b93e-139f81909106/d40a4eef5f7d63013a1f5bd8a1265616e6ddd63b #2742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rebase of #56 (implement clone/free) onto #81 (graph refactor) #83

rebase of #56 (implement clone/free) onto #81 (graph refactor) #83

carlobaldassi commented Dec 17, 2017 •

edited

Loading

carlobaldassi commented Dec 18, 2017

StefanKarpinski commented Dec 18, 2017

carlobaldassi commented Dec 18, 2017

StefanKarpinski commented Dec 18, 2017

KristofferC commented Dec 19, 2017

carlobaldassi commented Dec 21, 2017

carlobaldassi commented Jan 6, 2018

KristofferC commented Jan 6, 2018

andyferris commented Jan 14, 2018

andyferris commented Jan 14, 2018

KristofferC commented Jan 14, 2018

StefanKarpinski left a comment

StefanKarpinski Jan 14, 2018

KristofferC Jan 29, 2018

StefanKarpinski Jan 14, 2018

StefanKarpinski Jan 14, 2018

StefanKarpinski Jan 14, 2018

KristofferC Jan 29, 2018

StefanKarpinski Feb 14, 2018

StefanKarpinski Jan 14, 2018

StefanKarpinski commented Jan 14, 2018

andyferris commented Jan 15, 2018

StefanKarpinski commented Jan 15, 2018

andyferris commented Jan 15, 2018

carlobaldassi Jan 29, 2018

KristofferC Jan 29, 2018

carlobaldassi Jan 29, 2018

KristofferC Jan 29, 2018

KristofferC Jan 29, 2018

carlobaldassi Jan 29, 2018

KristofferC commented Mar 12, 2018

rebase of #56 (implement clone/free) onto #81 (graph refactor) #83

rebase of #56 (implement clone/free) onto #81 (graph refactor) #83

Conversation

carlobaldassi commented Dec 17, 2017 • edited Loading

carlobaldassi commented Dec 18, 2017

StefanKarpinski commented Dec 18, 2017

carlobaldassi commented Dec 18, 2017

StefanKarpinski commented Dec 18, 2017

KristofferC commented Dec 19, 2017

carlobaldassi commented Dec 21, 2017

carlobaldassi commented Jan 6, 2018

KristofferC commented Jan 6, 2018

andyferris commented Jan 14, 2018

andyferris commented Jan 14, 2018

KristofferC commented Jan 14, 2018

StefanKarpinski left a 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 14, 2018

andyferris commented Jan 15, 2018

StefanKarpinski commented Jan 15, 2018

andyferris commented Jan 15, 2018

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

KristofferC commented Mar 12, 2018

carlobaldassi commented Dec 17, 2017 •

edited

Loading