elves / elvish

Powerful scripting language & versatile interactive shell
https://elv.sh/
BSD 2-Clause "Simplified" License
5.65k stars 299 forks source link

Add comment why os:chmod tests might fail #1753

Closed krader1961 closed 8 months ago

krader1961 commented 8 months ago

Fixes #1748

xiaq commented 8 months ago

Thanks for bringing the issue to my attention.

The PR has some issues:

We should also simply fix the issue - as shown in https://go-review.googlesource.com/c/go/+/41430/5/src/cmd/go/internal/work/build_test.go, doing chgrp on the parent of the directory that one wishes to call chmod on does the trick.

krader1961 commented 8 months ago

The problem on my FreeBSD VM was because when I installed the OS it didn't put me in the "wheel" group. And the /tmp mount point was owned by the "wheel" group with the "sticky" attribute. I fixed it by adding my account to that group. Doing chgrp on /tmp is not a viable solution to this failure mode. The comment my change introduced was meant to only provide a hint to a future developer who might wonder why this unit test was failing.

The functional change to the unit test was only meant to make it obvious that the two tests operated on different directories. There are quite a few unit tests which do the same thing because it doesn't require the reader to recognize that TestWithSetup runs each test in a distinct temp dir and thus reduces the cognitive load when interpreting the test code.

krader1961 commented 8 months ago

I see your fix changes the ownership of the created temp directory rather than the parent of the created temp dir. I misunderstood your previous comment as implying the ownership of /tmp should be changed. Still, the comment in your fix is not very clear about the nature of the "Work around a quirk of FreeBSD's mkdir". I removed the if runtime.GOOS == "freebsd" { statement and the unit test passed on every platform I use for testing these things (macOS, Windows, FreeBSD, and several Linux distros). In other words, there is no reason to predicate the os.Chown on the OS being FreeBSD, AFAICT. The chown is a nop on non FreeBSD platforms.

xiaq commented 8 months ago

Sure, done in 464cfb64c64a9f61fbb1fe48ec43078860492960.