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

Bootstrap Pkg3 and add it to stdlib #25714

Closed
wants to merge 2 commits into from
Closed

Bootstrap Pkg3 and add it to stdlib #25714

wants to merge 2 commits into from

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Jan 23, 2018

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

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

stdlib/Makefile Outdated
PKG3_URL := https://github.com/JuliaLang/Pkg3.jl

Pkg3:
git clone $(PKG3_URL) $@
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

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.

stdlib/Makefile Outdated
default: get

PKG3_VERSION := f4139648bfb4cdde34e50947a73f71c7ee8d29a5
PKG3_URL := https://github.com/vchuravy/Pkg3.jl
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually where it's going to live?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

stdlib/Makefile Outdated
rm -rf Pkg3

SHA_VERSION := 8f2c9bb537097b661ac91580d18d20af51e5af22
SHA_URL := https://github.com/vchuravy/SHA.jl
Copy link
Member

Choose a reason for hiding this comment

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

Same question here

Copy link
Member Author

Choose a reason for hiding this comment

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

No this is just my fork that drops the Compat requirement

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!
Copy link
Member

Choose a reason for hiding this comment

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

Should these lines be deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -85,7 +85,10 @@ custom METADATA setup.
init(meta::AbstractString=DEFAULT_META, branch::AbstractString=META_BRANCH) = Dir.init(meta,branch)

function __init__()
Base.PKG_MODULE_REF[] = Pkg
Copy link
Member

Choose a reason for hiding this comment

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

Gross, I hope this goes away eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here :)
cc @KristofferC

stdlib/Makefile Outdated
Pkg3:
git clone $(PKG3_URL) $@
Pkg3/$(PKG3_VERSION).commit: Pkg3
git -C $< checkout $(PKG3_VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I will do that!

@vchuravy vchuravy changed the base branch from master to kc/excise_libgit2 January 24, 2018 00:22
@@ -0,0 +1,34 @@
default: get
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@vchuravy
Copy link
Member Author

@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.

Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

vtjnash commented Jan 24, 2018

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

github clone requires a branch name

@staticfloat
Copy link
Member

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 vchuravy changed the base branch from kc/excise_libgit2 to master February 8, 2018 21:44
@vchuravy
Copy link
Member Author

vchuravy commented Feb 8, 2018

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))))


extract-pkg3: $(BUILDDIR)/$(PKG3_SRC_DIR)/source-extracted
Pkg3: $(BUILDDIR)/$(PKG3_SRC_DIR)/source-extracted
cp -r $(BUILDDIR)/$(PKG3_SRC_DIR) $@
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@vchuravy
Copy link
Member Author

vchuravy commented Feb 9, 2018

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

@vchuravy
Copy link
Member Author

vchuravy commented Feb 9, 2018

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.

@vchuravy vchuravy closed this Feb 9, 2018
@StefanKarpinski
Copy link
Member

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.

@vchuravy vchuravy deleted the vc/pkg3_v2 branch August 19, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants