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

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

Open
Psykar opened this issue Apr 16, 2019 · 29 comments

Comments

@Psykar
Copy link

commented Apr 16, 2019

What did you do?

This has been discussed previously:
#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.

Psykar added a commit to Psykar/buildroot that referenced this issue Apr 16, 2019
Makefile: add chmod before rm when cleaning.
Some build systems (looking at you golang) create read only directories
as caches.
As such rm -rf will actually fail, causing clean and <pkg>-dirclean to fail.

This patch will cause `make clean` to run chmod -R +w on the relevant
directory first, which will allow rm -rf to work.

This may be resolved if golang/go#31481 is
resolved satisfactorily.

Signed-off-by: Louis des Landes <louis.deslandes@fleet.space>
Psykar added a commit to Psykar/buildroot that referenced this issue Apr 16, 2019
package/pkg-generic: add chmod when cleaning.
Some build systems (looking at you golang) create read only directories
as caches.
As such rm -rf will actually fail, causing clean and <pkg>-dirclean to fail.

This patch will cause `make <pkg>-dirclean` to force chmod -R +w on the relevant
directory first, which will allow rm -rf to work.

This may be resolved if golang/go#31481 is
resolved satisfactorily.

Signed-off-by: Louis des Landes <louis.deslandes@fleet.space>
Psykar added a commit to Psykar/buildroot that referenced this issue Apr 16, 2019
Makefile: add chmod before rm when cleaning.
Some build systems (looking at you golang) create read only directories
as caches.
As such rm -rf will actually fail, causing clean and <pkg>-dirclean to fail.

This patch will cause `make clean` to run chmod -R +w on the relevant
directory first, which will allow rm -rf to work.

This may be resolved if golang/go#31481 is
resolved satisfactorily.

Signed-off-by: Louis des Landes <louis.deslandes@fleet.space>
Psykar added a commit to Psykar/buildroot that referenced this issue Apr 16, 2019
package/pkg-generic: add chmod when cleaning.
Some build systems (looking at you golang) create read only directories
as caches.
As such rm -rf will actually fail, causing clean and <pkg>-dirclean to fail.

This patch will cause `make <pkg>-dirclean` to force chmod -R +w on the relevant
directory first, which will allow rm -rf to work.

This may be resolved if golang/go#31481 is
resolved satisfactorily.

Signed-off-by: Louis des Landes <louis.deslandes@fleet.space>
@agnivade

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

#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.

@dmitshur dmitshur changed the title Build argument to disable readonly $GOPATH/pkg/mod cmd/go: Build argument to disable readonly $GOPATH/pkg/mod Apr 16, 2019

@Psykar

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Marking it as a proposal.

@agnivade agnivade changed the title cmd/go: Build argument to disable readonly $GOPATH/pkg/mod proposal: cmd/go: build argument to disable readonly $GOPATH/pkg/mod Apr 17, 2019

@gopherbot gopherbot added this to the Proposal milestone Apr 17, 2019

@gopherbot gopherbot added the Proposal label Apr 17, 2019

@andybons andybons added the modules label Apr 17, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

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

This comment has been minimized.

Copy link

commented Apr 29, 2019

@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 bcmills removed the WaitingForInfo label Apr 29, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@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

This comment has been minimized.

Copy link

commented Apr 29, 2019

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 bcmills removed the WaitingForInfo label May 7, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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

This comment has been minimized.

Copy link
Author

commented May 9, 2019

@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 comment has been minimized.

Copy link
Contributor

commented May 28, 2019

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 bcmills removed the WaitingForInfo label May 29, 2019

@bcmills bcmills modified the milestones: Proposal, Go1.14 May 29, 2019

@bcmills bcmills added the NeedsFix label May 29, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented May 29, 2019

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.

@gopherbot gopherbot removed the NeedsFix label May 30, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented May 30, 2019

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

This comment has been minimized.

Copy link
Member

commented May 30, 2019

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

@jayconrod

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

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

This comment has been minimized.

Copy link
Member

commented May 30, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

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

This comment has been minimized.

Copy link

commented Jun 6, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

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 #27161 (comment). 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

This comment has been minimized.

Copy link

commented Jun 7, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

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

This comment has been minimized.

Copy link

commented Jun 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

@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

This comment has been minimized.

Copy link

commented Jun 13, 2019

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/net@v0.0.0-20190404232315-eb5bcb51f2a3/.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

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@dmitshur See note about Solaris builder in previous comment.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@jasonkeene, as noted in #31481 (comment) I think the execution bit is an easier win. Would you mind filing a separate issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.