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

vendor, cmd/vendor: not clean on master #36851

Closed
dmitshur opened this issue Jan 28, 2020 · 25 comments · Fixed by sthagen/golang-go#138
Closed

vendor, cmd/vendor: not clean on master #36851

dmitshur opened this issue Jan 28, 2020 · 25 comments · Fixed by sthagen/golang-go#138
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@dmitshur
Copy link
Contributor

On latest master branch right now, the src/vendor directory is not clean. Its contents do not correspond 1:1 to the versions specified by the src/go.mod file.

Following the steps in src/README.vendor results in a non-zero diff:

go $ go version
go version devel +b13ce14c4a Tue Jan 28 20:26:36 2020 +0000 darwin/amd64
go $ cd src
src $ go mod tidy
src $ go mod vendor
src $ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    vendor/golang.org/x/sys/cpu/cpu_riscv64.go

no changes added to commit (use "git add" and/or "git commit -a")

This is due to recent commits such as CL 216260, CL 216261, CL 216262 that are related to riscv64.

This makes it difficult and error prone to make changes to versions in src/go.mod, because the end result includes unexpected changes. See discussion in CL 216680.

If it's reasonable to resolve this, I don't think we should release Go 1.14 with a vendor directory that isn't clean, so tentatively marking this as release-blocker, but open to discussion.

/cc @4a6f656c @FiloSottile @golang/osp-team

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jan 28, 2020
@dmitshur dmitshur added this to the Go1.14 milestone Jan 28, 2020
@bcmills
Copy link
Contributor

bcmills commented Jan 28, 2020

If there are other x/sys changes that we don't want to fold in at this point, one low-risk option would be to commit an x/sys branch that is the same as the current commit but adds the new risc-v file, then update to that commit.

@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2020

I noticed today that src/cmd/vendor is not clean either. (It includes changes in both x/sys and x/tools).

@bcmills bcmills changed the title src/vendor: not clean on master vendor, cmd/vendor: not clean on master Jan 29, 2020
@dmitshur
Copy link
Contributor Author

dmitshur commented Jan 30, 2020

/cc @bcmills Do you think we should block Go 1.14 RC 1 on this? This is one of the potential blockers we are reviewing in the release meeting.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2020

I think it is important to at least decide how we will address this issue before we release RC1.

There appear to have been a large number of changes in x/sys/cpu and x/sys/unix in particular since those dependencies were last updated in the main repo.

The options that I see are:

  1. Release Go 1.14 with non-clean vendor directories.

  2. Upgrade the dependencies in question to some commit that covers the changed files (golang.org/x/sys v0.0.0-20200106114638-5f8ca72cd632 for CL 213397 and golang.org/x/tools v0.0.0-20200124162111-628e9aa307e6 for CL 216337).

  3. Publish minimal patches in an off-master branch to the sys and tools repos and update the std and cmd dependencies to point to the patched commits.

I don't particularly like option (1). Option (2) seems cleanest from a process perspective, but option (3) seems less risky.

I think only option (2) would need to gate an RC.

@dmitshur
Copy link
Contributor Author

dmitshur commented Jan 30, 2020

Thanks @bcmills.

If we release Go 1.14 with non-clean vendor directories, it will have an effect on all future Go 1.14.x minor releases (and security releases). It will increase the difficulty and risk when backporting future fixes to the release-branch.go1.14 branch of the main Go repo.

We'll either need to resolve this issue soon after releasing Go 1.14 (before Go 1.14.1, etc.), or bear that cost for the rest of the time that Go 1.14 release is supported (at least 1 year). If we're going to have to fix it anyway, it seems better and easier to do so before releasing Go 1.14.

As a result, I believe we should not go with option 1.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2020

Thinking about future backports, we will probably want a release-branch.go1.14 for the x/sys and x/tools repos anyway. That would fit pretty naturally with option (3), where the off-master branch is simply release-branch.go1.14.

(It's unfortunate to cut the release branch from such old base commits, but it seems a bit late to correct that particular mistake now.)

@FiloSottile
Copy link
Contributor

I agree that this close to the RC the best option is (2) with a new release branch.

That's what x/ repo release branches are for anyway, IMHO. See #36882.

@gopherbot
Copy link

Change https://golang.org/cl/217103 mentions this issue: golang.org/x/sys: update to v0.0.0-20200106114638-5f8ca72cd632

@ianlancetaylor
Copy link
Contributor

Just to see what would happen I tried updating x/sys in the main vendor directory.

I'm also fine with creating special purpose branches in x/sys and friends.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2020

From what I can tell, updating x/sys in the std module wouldn't be terrible — there are a few potentially-unnecessary changes, but nothing huge.

The x/sys/unix and x/tools changes that would be pulled into the cmd module, on the other hand, are fairly extensive.

@FiloSottile
Copy link
Contributor

Are the std and cmd versions in sync, ora can they at least be brought in sync with little damage? It would be pretty annoying to have two release branches, one for std and one for cmd.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2020

std doesn't depend on x/tools at all, and it's less than a month ahead on x/sys. So I think it should be fine to upgrade x/sys in cmd to the same baseline as what is already in std.

@gopherbot
Copy link

Change https://golang.org/cl/217060 mentions this issue: cmd: update x/sys version to match std

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jan 30, 2020

I folded the cmd update into my CL.

There are quite a few changes. They are probably fine, but I guess I would also be more comfortable with the branch approach. Does someone want to work on that? I'm not too sure how to do it myself.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2020

I sent CL 217060 to do the minimal update to get std and cmd on the same baseline. It's still a bigger diff than I expected, though.

@FiloSottile
Copy link
Contributor

I can do the branching, cherry-picking and vendoring, once we resolve how to get std and cmd in sync.

@gopherbot
Copy link

Change https://golang.org/cl/217118 mentions this issue: cmd/go.mod: sync x/crypto with std

gopherbot pushed a commit that referenced this issue Jan 30, 2020
    go get golang.org/x/crypto@v0.0.0-20200128174031-69ecbb4d6d5d
    go mod vendor
    git checkout -- vendor/golang.org/x/sys/unix/asm_linux_riscv64.s \
        vendor/golang.org/x/tools/go/analysis/passes/asmdecl/asmdecl.go

Updates #36851

Change-Id: I95c0584ede599f600da927a04f135fe64a85037e
Reviewed-on: https://go-review.googlesource.com/c/go/+/217118
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@4a6f656c
Copy link
Contributor

(1) obviously has more implications/fallout than planned, (2) was avoided since it potentially creates churn/breakage close to release, (3) makes sense - if someone can create the branches and/or provide names for these (we need x/sys for cpu/cpu_riscv64.go and unix/asm_linux_riscv64.s, and x/tools for go/analysis/passes/asmdecl), I can readily handle the code changes (or I can review them).

@4a6f656c
Copy link
Contributor

4a6f656c commented Jan 31, 2020

It's also worth noting that x/sys is not in sync across vendor and cmd/vendor:

golang.org/x/sys v0.0.0-20190529130038-5219a1e1c5f8 // indirect
golang.org/x/sys v0.0.0-20190502175342-a43fa875dd82 // indirect

Hopefully we can update cmd/vendor to 5219a1e to avoid having to have multiple branches here...

@gopherbot
Copy link

Change https://golang.org/cl/217218 mentions this issue: cmd/internal/moddeps: check for consistent versioning among all modules in GOROOT

@FiloSottile
Copy link
Contributor

This is the current state of the std and cmd modules. Only the vendored version matter because the other modules don't provide any packages that might need backporting.

$ grep '# golang.org/x/' vendor/modules.txt
# golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d
# golang.org/x/net v0.0.0-20191126235420-ef20fe5d7933
# golang.org/x/sys v0.0.0-20190529130038-5219a1e1c5f8
# golang.org/x/text v0.3.3-0.20191031172631-4b67af870c6f
$ grep '# golang.org/x/' cmd/vendor/modules.txt
# golang.org/x/arch v0.0.0-20190815191158-8a70ba74b3a1
# golang.org/x/crypto v0.0.0-20200128174031-69ecbb4d6d5d
# golang.org/x/mod v0.2.0
# golang.org/x/sys v0.0.0-20190502175342-a43fa875dd82
# golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e
# golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898

I created the following branches:

  • x/crypto release-branch.go1.14 at 69ecbb4d6d5d
  • x/net release-branch.go1.14 at ef20fe5d7933
  • x/sys release-branch.go1.14-std at 5219a1e
  • x/sys release-branch.go1.14-cmd at a43fa87
  • x/text release-branch.go1.14 at 4b67af8
  • x/arch release-branch.go1.14 at 8a70ba74b3a1
  • x/mod release-branch.go1.14 at ed3ec21bb8e
  • x/tools release-branch.go1.14 at 298f0cb
  • x/xerrors release-branch.go1.14 at 1b5146add898

Next I made the cherry-picks that had been applied manually:

Once those are merged I will update the go.mods and hopefully the vendor will be clean.

@ianlancetaylor
Copy link
Contributor

Thanks @FiloSottile !

@gopherbot
Copy link

Change https://golang.org/cl/217305 mentions this issue: std,cmd: sync go.mod with new release branches

@gopherbot
Copy link

Change https://golang.org/cl/217306 mentions this issue: [release-branch.go1.14-std] cpu: restore doinit() in cpu_riscv64.go

gopherbot pushed a commit to golang/sys that referenced this issue Feb 1, 2020
We cherry-picked this file to this branch which doesn't have CL 206859.
Rather than cherry-picking that CL too, add the doinit() function to
match what's currently in the vendor tree.

See also this comment on the go tree CL.
https://go-review.googlesource.com/c/go/+/216261/3#message-97b8f6687e7cfebc0403a35af8930a4e8948dff0

Updates golang/go#36851

Change-Id: Iddc40e989e56ea934e0aae203108373d5f90e92c
Reviewed-on: https://go-review.googlesource.com/c/sys/+/217306
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Feb 19, 2020
…es in GOROOT

Updates #36851
Fixes #36907

Change-Id: I29627729d916e3b8132d46cf458ba856ffb0beeb
Reviewed-on: https://go-review.googlesource.com/c/go/+/217218
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/236600 mentions this issue: cmd/internal/moddeps: don't skip directories if there are unusual files

gopherbot pushed a commit that referenced this issue Jun 8, 2020
Previously, if there was a non-directory file with the name vendor or
testdata in the Go source tree, it was possible for some directories
to be skipped by filepath.Walk performed in findGorootModules.

As unusual and unlikely as such non-directory files are, it's better
to ensure all directories are visited, and all modules in the GOROOT
source tree are found.

This increases confidence that tests relying on findGorootModule
will not have unexpected false negatives.

For #36851.
For #36907.

Change-Id: I468e80d8f57119e2c72d546b3fd1e23c31fd6e6c
Reviewed-on: https://go-review.googlesource.com/c/go/+/236600
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants