Go: proposal: cmd/go: build argument to disable readonly $GOPATH/pkg/mod

[Psykar]

What did you do?

This has been discussed previously:
https://github.com/golang/go/issues/27161

Most build systems (buildroot et al.) expect to be able to do an rm -rf to do a completely fresh build. However, because the pkg sets it's containing directories to read only, this command will fail.

Requiring a custom command to be added to various Makefile's scattered around the place to clean this up is far from ideal.

A potential solution might be to add an environment variable to disable this read only permission change on the pkg directory.
Open to other alternatives - but ideally there needs to be a command on the BUILD side that allows rm -rf to work correctly.

[agnivade]

27161 clearly mentions that it is intentional and will remain this way.

It would be good if your proposal addresses the issue raised by @rsc here.

[Psykar]

@agnivade - the default being read only makes complete sense to me, I'm not suggesting changing that.
I would have thought a manual flag to allow the directories to be writeable would be acceptable - to tell golang that I know what I'm doing, it's OK to not lock the directory down?

It's worth noting in this kind of scenario (embedded build systems), tests are not going to be run as part of the build process, and the whole build directory (which includes this cache directory) frequently gets removed entirely.

So we're looking for a way to allow rm -rf to work on the $GOPATH/pkg/mod directory while still blocking tests from being able to write files to this directory? The two seem mutually exclusive to me unfortunately, hence suggesting the manual flag.

(As an aside, happy to do the PR for this if it's acceptable)

[agnivade]

Marking it as a proposal.

[bcmills]

You can already run chmod u+w -R $GOPATH/pkg/mod or go clean -modcache explicitly before cleaning up.

Why would an explicit argument be more helpful than an explicit command? Seems like you have to update your configuration either way.

[pbrkr]

@bcmills: The problem for us is that we can't modify our CI system to run those commands at the start of a build. The fault here is 100% with the golang compiler, it has no valid case for removing the write bit from files & directories in the build tree.

Running git clean -xffd (which the CI system does) should be sufficient to fully clean up a build directory.

[bcmills]

@paul-betafive, how are you able to add arguments to the go command but not able to add a chmod or go clean command? Could you give some more detail on the specifics of your CI system?

[pbrkr]

We can add a chmod command after the go build command but that isn't water-tight as the build may fail or crash during go build after directories have already been marked as read-only. If the build server loses power or has a hard crash during go build then no error handling code will be able to run the chmod command.

We're using GitLab CI which unconditionally uses git clean after checkout to remove old build files. There's no option to add commands between the checkout and the clean.

[bcmills]

We're using GitLab CI which unconditionally uses git clean after checkout to remove old build files. There's no option to add commands between the checkout and the clean.

Wait, why is your module cache in the same git repository as the thing you're testing in CI? That seems like a very unusual configuration, and if you split the two it would be a lot easier to decouple the go clean -modcache or chmod from the go build or git clean step.

[Psykar]

@bcmills In my case at least it's not - buildroot (for example) build's it's output in a subdirectory of the repo however (which is under .gitignore)
Builds need to be reproducable without relying on any system packages (part of the build process will download golang source, compile and install it to a subdirectory, then use this installation for compiling the actual go packages which trigger this issue)

Having the module cache exist outside of the build directory means the build can potentially be affected by code outside of the repository.

For CI to a certain extend I agree that having the module cache actually be cached across builds makes a certain amount of sense.
But for buildroot (and similar) use case it does not.

FWIW the other side of the fence doesn't want to have to add a new command to their Makefile's specifically to handle a single package (golang) doing this.
http://lists.busybox.net/pipermail/buildroot/2019-April/248069.html

I'd like to find somewhere in the middle if possible!

[rsc]

This has come up enough that I think we should do something. There are good reasons for the default, but there are good reasons to be able to override it too. I suggest -pkgmodrw. CI systems can do things like export GOFLAGS=-pkgmodrw before their usual go commands.

/cc @bcmills @jayconrod

[bcmills]

I'm a little reluctant to put -pkgmod right there in the flag name, since it's conceivable that we'll want to move the package cache somewhere (outside of GOPATH?) in a future release.

That said, I'm still not sure why export GOFLAGS=-pkgmodrw is particularly better than go clean -modcache: either way, you end up needing to inject some sort of declaration at the start of the build.

[bcmills]

If we do add this flag, we should at least keep the _files_ themselves read-only: that would work around the rm -rf failure, but avoid at least some corruption due to stray edits (particularly from folks using editor “jump to definition” hooks).

[bcmills]

How about -rwmoddir? That would emphasize that only the directories are read-write.

[jayconrod]

I'm hesitant to have a flag for this for a couple reasons.

If we can agree on a single behavior that works more broadly, that would be preferable to more configuration. Making directories writable but files read-only would make rm -rf behave correctly and would still prevent accidental modification of source files. I'd like to understand cases where read-only directories prevent bugs though. If that behavior is rarely useful, then let's just have read-write directories. @bcmills mentioned this encourages tests to put temp files in temp directories. Anything else?

If we decide to make this configurable, a flag strikes me as the wrong interface. One could easily end up with a mix of read-only and read-write directories. I think it would make more sense to run a command that sets a policy and changes permissions on existing directories. The policy would apply to future commands. This might also make sense for things like explicitly set cache size limits.

That probably needs a bit more thought. GOFLAGS and go env -w already provide persistent configuration.

[bcmills]

I'd like to understand cases where read-only directories prevent bugs

In addition to preventing tests from making (potentially racy) writes to shared directories, they also prevent tools (such as go generate) from adding source files after-the-fact. That's important for reproducibility: if a package requires users to run go generate, that introduces a strong possibility that the generated results will differ (for example, by reading C headers that differ, or by invoking a different version of the generator tool, or running the tool in an environment that changes its behavior).

[rsc]

I think the part about stopping tests from scribbling in what should be immutable directories is very important. They need to run in those directories to open relative paths like "testdata/x.txt". But it's 555 directory that is causing the problem with rm. I don't see a way around that other than a flag.

[bcmills]

I don't see a way around that other than a flag.

Another possibility might be to make the directory tree containing a given module unwritable while running tests from that module, and chmod the directories back to read/write after the tests complete.

(But that would still leave us in a weird state if the go test process itself is killed midway through a run, and that's arguably worse than a flag.)

[jayconrod]

I'm convinced this needs to be configurable.

After thinking about configuration more, I don't think it makes sense to introduce any new configuration mechanism for this flag or other cache policies. GOFLAGS and go env -w are simple and should be good enough.

[ddevault]

I think this flag ought to be the default. Having the cache read-only is more nuisance than it is helpful, and not how any other software I'm aware of behaves.

To quote myself on #27161:

People don't go around rm -r'ing things unless they mean to, and even so - it's just a cache. These files are not important.

[bcmills]

People don't go around rm -r'ing things unless they mean to, and even so - it's just a cache. These files are not important.

Go back and read the discussion, particularly the golang-dev thread and https://github.com/golang/go/issues/27161#issuecomment-433102620. The point of making the cache non-writable is to prevent tests from (usually unintentionally) modifying the contents of the modules in which they run, not to prevent people from deleting the cache. rm -rf is just collateral damage; nobody set out to break it intentionally.

(Arguably rm -rf itself is buggy, in that it fails to actually remove things that are within its power to remove, but it wouldn't be realistic for us to expect that to change in the foreseeable future.)

[ddevault]

Right, and that makes sense. But why not make it read/write once the tests complete? Programs that go around literring read-only files on my disk are not good citizens of Unix.

[bcmills]

why not make it read/write once the tests complete?

The module cache is a cache: it may be shared among many concurrent go command invocations. “Once the tests complete” is a complicated condition to detect for any given path, and even then the directory could be left unwritable if the last go process is killed before it finishes updating permissions.

Moreover, we've found filesystem operations to be a significant bottleneck (for example, #28739), so avoiding unnecessary churn in the filesystem seems fairly important.

[RJPercival]

I don't see this mentioned so far but this behaviour also strips the executable bit from files, e.g. shell scripts, which isn't ideal. Presumably the intention was only to make them read-only?

[bcmills]

@RJPercival, I don't think we had even considered the possibility that the executable bit would need to be set, since the module cache is mainly only used for Go source code. But of course it's possible that a test of some package stored in the build cache will itself invoke a shell script, so we should probably preserve it.

I don't think we need a flag for that one: as long as the executable bit doesn't change hashes (and I'd be very surprised if it did), I think we should preserve the executable bit unconditionally, at least for non-.go source files.

[rorth]

I happen to admin the host that runs the solaris-oracle-amd64-oraclerel builder. The previous builder
admin is gone and I'm trying to decide how to continue. While having a Solaris builder is certainly
valuable, it must be as low-maintenance as possible, which it currently isn't due to this issue. Every once
in a while (usually several times a ay) the builder gos into a tight loop

2019/06/13 13:04:27 buildlet starting.
2019/06/13 13:04:27 remove /tmp/workdir-host-solaris-oracle-amd64-oraclerel/gopath/pkg/mod/golang.org/x/[email protected]/.gitattributes: permission denied
[ 2019 Jun 13 13:04:27 Stopping because all processes in service exited. ]

I needed Ian Taylor to point me at this Issue for some background on what's going on. IMO such a
builder needs to work out of the box, which due to this problem it currently doesn't. Having each builder
admin having to figure out what's going on and how to work around this seems a total waste of time to
me. Currently I don't even have an idea where to look for installing a workaround.

I compare this to the gdb buildbot running on the same systems: after initial setup is has been exactly
zero-maintenance, exactly as it should be. Everything else with golang builders is a considerable
disservice to the project, IMO.

[ianlancetaylor]

@dmitshur See note about Solaris builder in previous comment.

[dmitshur]

@rorth Thanks for reporting. I agree that having to manually intervene several times a day to keep a builder running is too much overhead and not sustainable. We should fix that.

I suspect we can fix it on the builder/buildlet side, and we may not need this proposal in the cmd/go command to be implemented to do so. Would you mind please filing a separate issue (or pointing to an existing one, if it already exists) for the builder issue you're experiencing with some more information, so we can investigate and look for a solution? Thanks.

[jasonkeene]

@RJPercival @bcmills I've ran into the execution bit problem as well. The k8s.io/code-generator module requires the execution bit to be set on their shell scripts for doing codegen. When using modules, this is stripped both from the mod cache and the vendor/ tree. I worked around this by just setting the execution bit before anything runs the codegen scripts and unsettling it after. It is silly and I've noticed others run into this as well and were disappointed with my work around. This is yet another mismatch from pre-modules behavior that will annoy people and raise the bar for adoption. I'm not sure if this is the place to discuss this but I would very much like to see the execution bits preserved.

[bcmills]

@jasonkeene, as noted in https://github.com/golang/go/issues/31481#issuecomment-500415155 I think the execution bit is an easier win. Would you mind filing a separate issue for that?

[bcmills]

Regarding the flag name: I would prefer something that does not assume that the path to the module cache always contains the string pkg/mod. At some point, we may want to change the location of the module cache or make it configurable (as in #34527).

So I would really rather we find a name other than -pkgmodrw.

How about -modcacherw, or perhaps -modcachemask=0777?

[rsc]

-modcacherw is fine. Let's not bring Unix mode bits into this.

[gopherbot]

Change https://golang.org/cl/202079 mentions this issue: cmd/go: add a flag to avoid creating unwritable directories in the module cache

[zx2c4]

Thank you for working on this. Having to chmod things is indeed a huge hassle and sometimes not available in all build systems.

Does this mean I'll need to pass -modcacherw to both go get and go build invocations, just in case the latter invokes the former? If so, would we be better served by making this nob into an environment variable?

[thepudds]

If so, would we be better served by making this nob into an environment variable?

My expectation/ guess is export GOFLAGS=... would work for you once this is merged:

https://golang.org/cmd/go/#hdr-Environment_variables

[bcmills]

Exactly: we expect that it will generally be used in conjunction with GOFLAGS.

[gopherbot]

Change https://golang.org/cl/202563 mentions this issue: cmd/go: support -modcacherw in 'go mod' subcommands