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

stdlib-git: improve integration with git-external #30075

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 18, 2018

Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.

@@ -1,2 +1,2 @@
PKG_BRANCH = master
PKG_SHA1 = d305e82fd353cb67e8a064800b9972ee1cb7b5e0
PKG_SHA1 = f9180e48b27a843aeee864db814dce57cb296b9b
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changing the Pkg sha?

Copy link
Member Author

Choose a reason for hiding this comment

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

It had the wrong SHA1 from what was intended here. This is now the whole commit hash, while previously it only contained the tree hash for this commit. This var is (intentionally) allowed to specify any git object (incl. branch, tag, etc.), but typically it should have the commit SHA, since that's the most general and easiest to work with (e.g. what git usually presents you).

Copy link
Member

Choose a reason for hiding this comment

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

But why are the checksums also changed? Shouldn't the commit you set here produce the same tarball?

Copy link
Member Author

Choose a reason for hiding this comment

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

The name here gets inserted by GitHub as part of the tarball file contents (JuliaLang-Pkg.jl-f9180e4/ vs. JuliaLang-Pkg.jl-d305e82/)

@fredrikekre fredrikekre added the stdlib Julia's standard library label Nov 19, 2018
Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.
SRCDIR := $(abspath $(dir $(lastword $(MAKEFILE_LIST))))
JULIAHOME := $(abspath $(SRCDIR)/..)
SRCCACHE := $(abspath $(SRCDIR)/srccache)
BUILDDIR := $(SRCCACHE)
BUILDDIR := .
Copy link
Member

Choose a reason for hiding this comment

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

Can we not cache the Pkg-$COMMITSHA folders in SRCCACHE instead of poisoning /stdlib/?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want an out-of-tree build, you should do an out-of-tree build. Some Packages have a tendency to mutate their own contents (usually just a deps/ folder), so as a whole that probably more nearly makes them build artifacts than source code. But anyways, this variable is just documenting where build artifacts must go (which must not be the same as srccache where source inputs should go); actually deciding whether something is a shared, immutable input or a build artifact is up to the rest of the Makefile.

@@ -1,2 +1,2 @@
PKG_BRANCH = master
PKG_SHA1 = d305e82fd353cb67e8a064800b9972ee1cb7b5e0
PKG_SHA1 = f9180e48b27a843aeee864db814dce57cb296b9b
Copy link
Member

Choose a reason for hiding this comment

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

But why are the checksums also changed? Shouldn't the commit you set here produce the same tarball?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Nov 21, 2018

Related: #30088, in which the checksums are incorrect.

@vtjnash vtjnash merged commit ef50e25 into master Nov 26, 2018
@vtjnash vtjnash deleted the jn/stdlib-git branch November 26, 2018 15:17
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Nov 26, 2018

Just to clarify some things that were not clear to me but which I just got clear on talking with @vtjnash:

  1. The PKG_SHA1 variable changed from a tree hash to a commit hash with the same tree. This is because you can't git checkout a tree hash but you can do so with a commit and in git checkout mode, git-external uses this name to check things out.

  2. The tarball checksums changed because GitHub includes part of PKG_SHA1 in the prefix of paths in the tarball, so paths of things in the tarball changed even though they're otherwise the same.

@KristofferC
Copy link
Member

This seems to have broken things. Trying to add Pkg as a dependency gives:

(v1.1) pkg> add Pkg
  Updating registry at `~/.julia/registries/General`
  Updating git-repo `https://github.com/JuliaRegistries/General.git`
ERROR: The following package names could not be resolved:
 * Pkg (44cfe95a-1eb2-52ea-b672-e2afdf69b78f in manifest but not in project)
Please specify by known `name=uuid`.

I also got

  [44cfe95a] - Pkg
→ [44cfe95a] + Pkg-f9180e48b27a843aeee864db814dce57cb296b9b

after doing a Pkg command.

@vtjnash
Copy link
Member Author

vtjnash commented Nov 27, 2018

The Pkg package shouldn't be able to see this change, but it certainly might expose existing issues (either normalizing with realpath too soon, or looking at the wrong folder altogether).

FWIW, I don't see the issue you describe (although it theoretically would happen if you haven't deleted the usr/ folder in a month, since before the initial PR to move Pkg to be a git-external).

@KristofferC
Copy link
Member

The problem with having both Pkg and Pkg-f918.. is that right now there is an assumption that stdlib/StdLib defines a standard library StdLib. This is no longer the case.

@vtjnash
Copy link
Member Author

vtjnash commented Nov 27, 2018

stdlib is just a random folder, not really associated with anything. The standard library lives inside usr/, as with all build content. Specifically, it's at Sys.STDLIB = ".../usr/share/julia/stdlib/v1.1" (although if you haven't deleted usr/ in some weeks, the build system will be confused and gets them mixed up)

KristofferC pushed a commit that referenced this pull request Dec 6, 2018
Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.

(cherry picked from commit ef50e25)
@KristofferC KristofferC mentioned this pull request Dec 6, 2018
61 tasks
KristofferC pushed a commit that referenced this pull request Dec 12, 2018
Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.

(cherry picked from commit ef50e25)
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.

(cherry picked from commit ef50e25)
KristofferC pushed a commit that referenced this pull request Feb 20, 2020
Previously some of the work was shifted, leading to awkward extraneous copies
and preventing usage of DEPS_GIT=1 mode (the main point of this script).
There also seemed to be unexpected failure modes in the current usage
where the files in stdlib/Pkg would routinely go missing for me. I don't know
the source of those, but I hope that this will fix it (because it makes more of
the state immutable).

Also noticed some issues with existing scripts (expanding variables at
unintended points, not rebuilding on some changes), and fixed those.

(cherry picked from commit ef50e25)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants