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

Download Pkg from GitHub releases. #29615

Merged
merged 2 commits into from Oct 27, 2018

Conversation

10 participants
@fredrikekre
Copy link
Member

commented Oct 12, 2018

Hacking my way through Makefile land... at least this seems to work so
something

Edit: Linux only apparently...

@fredrikekre fredrikekre added the stdlib label Oct 12, 2018

@ararslan ararslan requested a review from staticfloat Oct 12, 2018

@staticfloat

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

I think the errors are related to a broken registry? Why would it be complaining about JSON?

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

Because the release of Pkg in this PR does not have everything needed for the tests to pass. We need to make a new Pkg release.

@vchuravy

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

Thanks for working on this!

In #25714 I separated the stdlib from the deps folder and used git-external to handle the download process which takes care of downloading the tarball or using git and managing the cache accordingly. The latter should be done here as well and the former is a question of preference. I personally would rather separate the notions of dependencies and stdlib packages.

Are you also thinking about after bootstrapping Pkg to use it to resolve a Manifest file for the stdlib? I personally think that would be neat, but it might require some gymnastics.

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

used git-external to handle the download

Ok, will change to this. What is the advantage?

separated the stdlib from the deps folder

Are you also thinking about after bootstrapping Pkg to use it to resolve a Manifest file for the stdlib?

Yes, so given this, is it worth separating stdlib out since it might only be Pkg that is installed with the Makefile and the rest of the stdlibs using Pkg?

$(eval $(call git-external,Pkg,PKG,,,$(BUILDDIR)))

extract-pkg: $(BUILDDIR)/$(PKG_SRC_DIR)/source-extracted
Pkg: $(BUILDDIR)/$(PKG_SRC_DIR)/source-extracted

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Oct 15, 2018

Author Member

Shouldn't this check for Pkg/source-extracted rather than the file in the build location?

This comment has been minimized.

Copy link
@staticfloat

staticfloat Oct 15, 2018

Member

$(call git-external,Pkg,PKG,,,$(BUILDDIR))

This tells git-external to download and extract it into $(BUILDDIR). See this line.

@vchuravy

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Yes, so given this, is it worth separating stdlib out since it might only be Pkg that is installed with the Makefile and the rest of the stdlibs using Pkg?

I would expect that buildstep using Pkg to grab new packages to also use the stdlib/Makefile. When I wrote this code the second time around I was hoping to remove the ext folder from Pkg3 and have the necessary buildsteps to bootstrap them for Pkg3. That's why I had SHA and Compat in there.

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

The only thing in ext is a TOML parser, we can move it to src if it matters...

Also, building things like Compat into the sysimg is a bad idea.

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2018

Compat isn't used by Pkg anymore so no need.

@vchuravy

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

I was just explaining my reasoning back then :) TOML itself would be useful to have outside Pkg as its own package.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

The TOML business is a bit complicated, let's deal with that later and leave it in ext for now.

@fredrikekre fredrikekre force-pushed the fe/Pkg branch 2 times, most recently from 0958fde to 1ae7a73 Oct 19, 2018

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

This seem to work now (at least on my machines, lets see what CI says...), so please review! Note that the only thing to review is the second commit, the first one just removes stdlib/Pkg. Before merging we should update the sha to an actual Pkg release instead of the current backport branch.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Why would we backport this to 1.0? You're kind of liberal with that label, @fredrikekre.

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Because we need the bugfixes from Pkg that this brings into 1.0.2?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

It seems a bit aggressive for a point release, but ok.

@vchuravy
Copy link
Member

left a comment

This doesn't work with out-of-dir builds e.g. make O=/tmp

LoadError("sysimg.jl", 491, ArgumentError("Package Pkg not found in current path:\n- Run `import Pkg; Pkg.add(\"Pkg\")` to install the Pkg package.\n"))
Makefile Outdated
@@ -59,6 +59,9 @@ endif
julia-deps: | $(DIRS) $(build_datarootdir)/julia/base $(build_datarootdir)/julia/test $(build_defaultpkgdir)
@$(MAKE) $(QUIET_MAKE) -C $(BUILDROOT)/deps

julia-stdlib:

This comment has been minimized.

Copy link
@vchuravy

vchuravy Oct 19, 2018

Member
Suggested change
julia-stdlib:
julia-stdlib: | $(DIRS)

Also need to be added in line 12

 BUILDDIRS := $(BUILDROOT) $(addprefix $(BUILDROOT)/,base src ui doc deps stdlib test test/embedding test/llvmpasses)
@vchuravy

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

 build_defaultpkgdir = $(build_datarootdir)/julia/stdlib/$(shell echo $(VERSDIR))
 $(eval $(call symlink_target,$(JULIAHOME)/stdlib,$(build_datarootdir)/julia/stdlib,$(shell echo $(VERSDIR))))

Is the culprit here, we are linking the entire stdlib to usr/share/julia/stdlib/v1.1. Instead of linking each individual stdlib from $(JULIAHOME/stdlib) and from $(BUILDROOT)/stdlib

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

It seems a bit aggressive for a point release, but ok.

Maybe, it could be delayed for 1.1 but then someone needs to find the time to re-backport the Pkg commits to Julia (or we just wait with them until Julia 1.1).

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

Why would we backport this to 1.0? You're kind of liberal with that label, @fredrikekre.

Because we need the bugfixes from Pkg that this brings into 1.0.2?

Right, so the idea was for this PR to be the equivalent to the "Bump Pkg" PRs we have made earlier for Julia v1.0.2. The manual syncing of the repos have not worked very well, and some important things have been missed here and there; for example JuliaLang/Pkg.jl#470 was missed for Julia 1.0 which has caused ~10 threads on discourse, and JuliaLang/Pkg.jl#634 missed Julia v1.0.1 which have resulted in even more threads/issues.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Ok, carry on but please do be careful with point releases.

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

This works for me locally with an out-of-source build now.

@fredrikekre

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

I have made a v1.0.2 release of Pkg and updated the tree-sha in this PR so this is good to go from my POV.

@KristofferC KristofferC merged commit 1717adb into master Oct 27, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@fredrikekre fredrikekre deleted the fe/Pkg branch Oct 27, 2018

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

🎉

@KristofferC KristofferC referenced this pull request Oct 29, 2018

Merged

WIP: Backports for 1.0.2 #29444

104 of 108 tasks complete

KristofferC added a commit that referenced this pull request Oct 29, 2018

Download Pkg from GitHub releases. (#29615)
* Remove Pkg from the JuliaLang/julia repo.

* Use git-external to install Pkg during build.

(cherry picked from commit 1717adb)

KristofferC added a commit that referenced this pull request Oct 31, 2018

Download Pkg from GitHub releases. (#29615)
* Remove Pkg from the JuliaLang/julia repo.

* Use git-external to install Pkg during build.

(cherry picked from commit 1717adb)

KristofferC added a commit that referenced this pull request Nov 2, 2018

Download Pkg from GitHub releases. (#29615)
* Remove Pkg from the JuliaLang/julia repo.

* Use git-external to install Pkg during build.

(cherry picked from commit 1717adb)
@@ -527,10 +527,11 @@ source-dist:
full-source-dist: light-source-dist.tmp
# Get all the dependencies downloaded
@$(MAKE) -C deps getall NO_GIT=1
@$(MAKE) -C stdlib getall

This comment has been minimized.

Copy link
@nalimilan

nalimilan Nov 10, 2018

Contributor

I guess this was supposed to include Pkg in the source tarballs? Apparently it didn't work: the 1.0.2 tarball doesn't contain it. That's annoying for building distribution packages without network access.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Nov 10, 2018

Author Member

Hmm, I'm 90% sure I tried this, but maybe I didn't do it from a clean repo. We should be able to re-upload the source tarballs with this included, @ararslan? I'll have a look at it.

This comment has been minimized.

Copy link
@ararslan

ararslan Nov 10, 2018

Member

Yes we can re-create and re-upload the source tarballs, it's just a bit annoying since we can't go through the full-source-dist Make target; we'll have to do everything manually. Apologies for not catching this during the release preparation.

This comment has been minimized.

Copy link
@nalimilan

nalimilan Nov 10, 2018

Contributor

I was talking about light-source-dist, but maybe full-source-dist has the same problem.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Nov 10, 2018

Author Member

Should it be included there? I interpreted https://github.com/JuliaLang/julia/blob/master/Makefile#L516 as only including code from this repo, and thus Pkg is just an external dependency that should only be in the full variant, which it seems to be btw?

This comment has been minimized.

Copy link
@nalimilan

nalimilan Nov 10, 2018

Contributor

Well, all other external dependencies are libraries that could be installed separately from Julia, and which generally exist as distribution packages. OTC Pkg cannot be used without Julia. Also, in practical terms, if the plan is to move progressively all stdlib modules to separate repos, it would be quite painful for packagers to have to handle all of them manually (and update versions one by one for each new release).

@timholy

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

For anyone who gets

LoadError("sysimg.jl", 491, ArgumentError("Package Pkg not found in current path:\n- Run `import Pkg; Pkg.add(\"Pkg\")` to install the Pkg package.\n"))

during building, you can fix it with

make -C stdlib clean-pkg

and then building again.

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

KristofferC added a commit that referenced this pull request Feb 11, 2019

Download Pkg from GitHub releases. (#29615)
* Remove Pkg from the JuliaLang/julia repo.

* Use git-external to install Pkg during build.

(cherry picked from commit 1717adb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.