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

x/exp/cmd/gorelease: ignore .gitignored files when compiling local .zip file #37413

Closed
myitcv opened this issue Feb 24, 2020 · 14 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Feb 24, 2020

What version of Go are you using (go version)?

$ go version
go version devel +151ccd4bdb Mon Feb 24 02:36:05 2020 +0000 linux/amd64
$ git rev-parse HEAD
7c80518d1cc79ffde9e571fe4fd281f321d36200

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build875262587=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ gorelease -base=v0.0.29

in a checkout of govim which is ~8MB in size at the time of writing.

What did you expect to see?

A comparison with v0.0.29 to be presented

What did you see instead?

gorelease: create zip /home/myitcv/gostuff/src/github.com/myitcv/govim: module source tree too large (max size is 524288000 bytes)

It appears that gorelease is including .gitignore-d files when creating a local zip file. It should almost certainly ignore those. Indeed gorelease should probably fail unless git status -porcelain is clean to avoid any ambiguity.

cc @jayconrod

@gopherbot gopherbot added this to the Unreleased milestone Feb 24, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 27, 2020
@jayconrod jayconrod changed the title x/exp/gorelease: ignore .gitignored files when compiling local .zip file x/exp/cmd/gorelease: ignore .gitignored files when compiling local .zip file Feb 28, 2020
@jayconrod jayconrod modified the milestones: Unreleased, gorelease May 25, 2021
@jeanbza
Copy link
Member

jeanbza commented Jun 4, 2021

It appears that gorelease is including .gitignore-d files when creating a local zip file. It should almost certainly ignore those.

@jayconrod We are currently using zip.CreateFromDir to create the zip file. Seems like this change should be in that function rather than gorelease, since gorelease doesn't really handle any of this logic. Does that SGTY?

Indeed gorelease should probably fail unless git status -porcelain is clean to avoid any ambiguity.

@myitcv Could you elaborate as to how this relates to the zip issue? Feels a bit like a separate FR here, one which I'm not sure is a bit more gray area: I could certainly see the value in running gorelease on uncommitted work to see if you've made changes that will require major version bumps.

@myitcv
Copy link
Member Author

myitcv commented Jun 5, 2021

Could you elaborate as to how this relates to the zip issue? Feels a bit like a separate FR here, one which I'm not sure is a bit more gray area: I could certainly see the value in running gorelease on uncommitted work to see if you've made changes that will require major version bumps.

I think reasoning at the time (and indeed still now) was that pseudoversions are defined in terms of commits, something that everyone has come to expect. If gorelease by default uses the working tree, that would be a surprise to my mind relative to the behaviour of cmd/go.

FWIW with unity (totally unrelated example, but has to solve exactly the same problem) the command fails if called with a non-porcelain tree, but the user can pass the -staged flag to use staged changes. (The choice of staged here is important because it's otherwise tricky to calculate the diff to apply).

FWIW this also has relevance as far as the "ignore" problem is concerned. unity builds its equivalent of the the zip file using a temporary git worktree of the current directory, overlaying the staged changes if -staged is passed, in order to sidestep the calculation of what should be ignored.

@jeanbza
Copy link
Member

jeanbza commented Jun 11, 2021

Sorry for the slow responses! (This is a Friday project for me)

I think reasoning at the time (and indeed still now) was that pseudoversions are defined in terms of commits, something that everyone has come to expect. If gorelease by default uses the working tree, that would be a surprise to my mind relative to the behaviour of cmd/go.

Could you elaborate further?

One can certainly run go build on uncommitted changes (that is: changes which are not committed, which would cause git status --porcelain to return a "non-clean" output). I would imagine people care to do the same for gorelease. A made-up example:

  • I make some change
  • I'm unsure if it constitutes a breaking change, so I won't commit it yet
  • gorelease
  • It tells me I made a breaking change that requires a major version bump. I'll undo my change

I suspect I'm not fully understanding your request, though - would love more information. 😃

@jayconrod
Copy link
Contributor

We are currently using zip.CreateFromDir to create the zip file. Seems like this change should be in that function rather than gorelease, since gorelease doesn't really handle any of this logic. Does that SGTY?

@jadekler I'm worried about this adding too much complexity and incompatibility to CreateFromDir. .gitignore could appear in any parent directory of the repository, possibly above the main module. We should also observe .git/info/exclude. We might want to support other VCS tools' concepts of ignored files, too.

This should probably be implemented in golang.org/x/mod/zip though. Perhaps in a new function, CreateFromVCS that accepts a repo directory, a revision ID (Git SHA1, tag, branch, etc.), and a subdirectory within the repo? It would be an error to invoke that function when there are uncommitted changes in the repo.

For reference, the ReadZip methods in cmd/go/internal/modfetch/codehost implement part of this, though I don't think we could use that code as-is. They use commands like git archive to produce zip files. Those files can't be used directly as module zips, since they can contain nested modules, and they have the wrong filename prefix, so cmd/go copies files out into a new module zip with x/mod/zip.Create.

@myitcv
Copy link
Member Author

myitcv commented Jun 21, 2021

Sorry for the delay, was on holiday.

Could you elaborate further?

I think my suggestion is perhaps more a function of an implementation restriction within unity that you probably don't suffer from (we need to copy the working tree to another directory... which gets tricky when you need to consider ignored files etc, hence we simplify by requiring changes that should be considered to be staged). So to that end, I think I agree with your position; that doing a diff relative to an arbitrary working tree should work.

@gopherbot
Copy link

Change https://golang.org/cl/330769 mentions this issue: zip: add CreateFromVCS, which creates a module zip from vcs

@gopherbot
Copy link

Change https://golang.org/cl/341930 mentions this issue: cmd/gorelease: use CreateFromVCS instead of CreateFromDir

gopherbot pushed a commit to golang/mod that referenced this issue Aug 13, 2021
Updates golang/go#37413

Change-Id: I5ea07a6e4eedc6cb215e4893944f1ab215ea8f2b
Reviewed-on: https://go-review.googlesource.com/c/mod/+/330769
Trust: Jean de Klerk <deklerk@google.com>
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jean de Klerk <deklerk@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@nightlyone
Copy link
Contributor

I don't think always ignoring files ignored by git is a good idea. It might be a great default, but I know people and projects where e.g. the files generated by protoc an such generators are not in the git repository. But those are necessary to publish the module as otherwise it would not compile.

@jeanbza
Copy link
Member

jeanbza commented Aug 27, 2021

@nightlyone Is your concern that .gitignore-ed files would not be published in a module? Running gorelease (and changing gorelease to ignore .gitignore files) does not actually publish a module: it just performs checks.

@nightlyone
Copy link
Contributor

@jadekler the Go code contained in the module might not even compile when excluding the files mentioned in the .gitignore file (e.g. foo.pb.go might be excluded, because the CI and developer should always generate them locally from foo.proro files).

@jayconrod
Copy link
Contributor

@nightlyone The approach we're taking here is to read out files that are checked into the Git repo at a particular commit. If files needed to build packages are not checked in, gorelease will report errors. I think that's probably the right behavior here; not sure how else it would work.

@gopherbot
Copy link

Change https://golang.org/cl/345730 mentions this issue: zip: add ErrUnrecognizedVCS error, allowing fallback behavior

gopherbot pushed a commit to golang/mod that referenced this issue Aug 30, 2021
Add ErrUnrecognizedVCS, which allows calling functions (such as gorelease) to
fallback: usually to CreateFromDir.

Updates golang/go#37413

Change-Id: I846f72b1ce22bfc699e8cd83b28ea4529e73d6e9
Reviewed-on: https://go-review.googlesource.com/c/mod/+/345730
Trust: Jean de Klerk <deklerk@google.com>
Run-TryBot: Jean de Klerk <deklerk@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@jeanbza
Copy link
Member

jeanbza commented Aug 31, 2021

@myitcv This involved quite a lot of code. I'm fairly sure this WAI now, but if you have some time, would love a double check things work as you expect. =)

@myitcv
Copy link
Member Author

myitcv commented Sep 3, 2021

@jadekler thanks for the ping on this! Looks like everything is working as expected, thanks!

@golang golang locked and limited conversation to collaborators Sep 3, 2022
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.
Projects
None yet
Development

No branches or pull requests

6 participants