Bootstrap Pkg3 and add it to stdlib by vchuravy · Pull Request #25714 · JuliaLang/julia

vchuravy · 2018-01-23T23:15:22Z

This is on top of #25706 and a continuation of #25140

move Pkg3/ext/{TOML, BinaryProvider, TerminalMenus} into its own packages excise ext/* Pkg.jl#118

~~This is based on my own forks that make SHA.jl work without Compat and deals with the Nullables removal for TOML.jl~~

I would appreciate a review by either @staticfloat or @vtjnash for the Makefile part

tkelman · 2018-01-23T23:18:57Z

stdlib/Makefile

+PKG3_URL := https://github.com/JuliaLang/Pkg3.jl
+
+Pkg3:
+	git clone $(PKG3_URL) $@


use the same mechanism as for C deps. git is not a build dependency.

staticfloat

Makefile and what Julia code I had time to read looks overall good, but we can't rely on git, we need to use the git-external tools.

staticfloat · 2018-01-23T23:23:37Z

stdlib/Makefile

+default: get
+
+PKG3_VERSION := f4139648bfb4cdde34e50947a73f71c7ee8d29a5
+PKG3_URL := https://github.com/vchuravy/Pkg3.jl


Is this actually where it's going to live?

No this is just where the changes are to make it actually work right now.

staticfloat · 2018-01-23T23:24:43Z

stdlib/Makefile

+	rm -rf Pkg3
+
+SHA_VERSION := 8f2c9bb537097b661ac91580d18d20af51e5af22
+SHA_URL := https://github.com/vchuravy/SHA.jl


Same question here

No this is just my fork that drops the Compat requirement

staticfloat · 2018-01-23T23:25:08Z

stdlib/Pkg/src/Pkg.jl

-       update, resolve, test, build, free, pin, PkgError, setprotocol!
+#export Dir, Types, Reqs, Cache, Read, Query, Resolve, Write, Entry
+#export dir, init, rm, add, available, installed, status, clone, checkout,
+#       update, resolve, test, build, free, pin, PkgError, setprotocol!


Should these lines be deleted?

You accidentally reviewed #25705 as well, sorry about that I should probably have pointed the PR at that branch to minimise the diff.
cc @KristofferC

staticfloat · 2018-01-23T23:42:18Z

stdlib/Pkg/src/Pkg.jl

 init(meta::AbstractString=DEFAULT_META, branch::AbstractString=META_BRANCH) = Dir.init(meta,branch)

 function __init__()
+    Base.PKG_MODULE_REF[] = Pkg


Gross, I hope this goes away eventually.

Same here :)
cc @KristofferC

staticfloat · 2018-01-23T23:49:05Z

stdlib/Makefile

+Pkg3:
+	git clone $(PKG3_URL) $@
+Pkg3/$(PKG3_VERSION).commit: Pkg3
+	git -C $< checkout $(PKG3_VERSION)


I think you need to use the tools defined in deps/tools/git-external.mk. Basically we define both a git url and a tarball url, so that if the user wants to use git they can, but if they want to just use curl and tar they can do that as well.

Thanks I will do that!

iblislin · 2018-01-24T01:32:04Z

stdlib/Makefile

@@ -0,0 +1,34 @@
+default: get


Could you tell more about this approach v.s. git submodule?

see #10743, submodules were used once upon a time for deps and it was pretty awful

vchuravy · 2018-01-24T02:31:08Z

@staticfloat switched to using git-external. One thing that I am not sure about if in this setup Pkg3 gets updated when the PKG3_SHA1 or the PKG3_BRANCH get updated and if I wired everything correctly.

@iblis17 I proposed git submodules originally and several people rightfully voiced reservations. The main problem in my opinion is that submodules a stateful and everyone needs to be aware how to maintain them, e..g. if you can easily forget a git submodule update after a git pull. So this uses the dependency resolution tooling that we already are using and when you switch branches with different versions, make should take care of updating everything to the right state.

staticfloat

Looks good to me! I also do not understand why we have the _BRANCH variables, as although we use them in initial clone/checkout, we always checkout $(PKG3_SHA1), so it doesn't seem necessary to me to have a branch name.

vtjnash · 2018-01-24T14:33:52Z

so it doesn't seem necessary to me to have a branch name.

github clone requires a branch name

staticfloat · 2018-01-24T16:37:40Z

I’m assuming you mean “git” clone and are not speaking about something that is specific to github. If that is true, I don’t think we need to specify a branch name, we could just let it check out whatever is the default branch, then checkout to our specific sha, right?

vchuravy · 2018-02-08T21:46:12Z

Predicated on JuliaCrypto/SHA.jl#53 and JuliaLang/Pkg.jl#117

Currently fails with:

WARNING: importing deprecated binding Base.Nullable into TOML.
LoadError("sysimg.jl", 860, LoadError("/data/vchuravy/julia/usr/share/julia/site/v0.7/Pkg3/src/Pkg3.jl", 41, LoadError("/data/vchuravy/julia/usr/share/julia/site/v0.7/Pkg3/src/../ext/TOML/src/TOML.jl", 12, TypeError(Symbol("Type{...} ex
pression"), "", UnionAll, Base.Nullable))))

vchuravy · 2018-02-08T21:48:35Z

stdlib/Makefile

+
+extract-pkg3: $(BUILDDIR)/$(PKG3_SRC_DIR)/source-extracted
+Pkg3: $(BUILDDIR)/$(PKG3_SRC_DIR)/source-extracted
+	cp -r $(BUILDDIR)/$(PKG3_SRC_DIR) $@


This cp does the wrong thing if the directory already exists.

vchuravy · 2018-02-09T03:53:13Z

Should this alongside #25324 be put on the 1.0 milestone?

vchuravy · 2018-02-09T16:28:16Z

I don't quite understand why #25953 was merged after triage decided against using git subtrees, and I still strongly oppose that approach.
But the floodgates are open, and this PR is now obsolete.

StefanKarpinski · 2018-02-09T17:09:38Z

There was some discussion afterwards and since @KristofferC and I are the ones who actually develop and maintain Pkg3 and both of us want to move it into julia/stdlib instead of maintaining it externally, we went the other way. All the people in favor of git externals aren't working on it, so their opinion is at best hypothetical. Pkg3 and Base are quite closely coupled and it just doesn't make sense at this point to have it be the only external package. We could in the future move to an arrangement where some stdlib packages have external repos, but Pkg3 should definitely not be the first one.

tkelman reviewed Jan 23, 2018

View reviewed changes

staticfloat requested changes Jan 23, 2018

View reviewed changes

vchuravy changed the base branch from master to kc/excise_libgit2 January 24, 2018 00:22

iblislin reviewed Jan 24, 2018

View reviewed changes

staticfloat approved these changes Jan 24, 2018

View reviewed changes

KristofferC force-pushed the kc/excise_libgit2 branch from 9accacc to 103e875 Compare January 24, 2018 15:21

KristofferC force-pushed the kc/excise_libgit2 branch 6 times, most recently from 494c570 to 5f909ec Compare January 31, 2018 12:09

KristofferC force-pushed the kc/excise_libgit2 branch from 5f909ec to bbf2224 Compare February 5, 2018 13:58

vchuravy mentioned this pull request Feb 8, 2018

use git subtree to graft Pkg3, etc. into stdlib #25942

Closed

vchuravy added 2 commits February 8, 2018 16:42

add Pkg3, Compat, SHA to stdlib

86509e6

only test stdlib directories that are proper packages

932fa0e

vchuravy force-pushed the vc/pkg3_v2 branch from 5096d58 to 932fa0e Compare February 8, 2018 21:43

vchuravy changed the base branch from kc/excise_libgit2 to master February 8, 2018 21:44

vchuravy commented Feb 8, 2018

View reviewed changes

vchuravy mentioned this pull request Feb 8, 2018

excise ext/* JuliaLang/Pkg.jl#118

Closed

vchuravy closed this Feb 9, 2018

vchuravy deleted the vc/pkg3_v2 branch August 19, 2018 15:22

vchuravy mentioned this pull request Oct 14, 2018

Download Pkg from GitHub releases. #29615

Merged

Uh oh!

Conversation

vchuravy commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchuravy commented Jan 24, 2018

Uh oh!

staticfloat left a comment

Choose a reason for hiding this comment

Uh oh!

vtjnash commented Jan 24, 2018

Uh oh!

staticfloat commented Jan 24, 2018

Uh oh!

vchuravy commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchuravy commented Feb 9, 2018

Uh oh!

vchuravy commented Feb 9, 2018

Uh oh!

StefanKarpinski commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vchuravy commented Jan 23, 2018 •

edited

Loading