Cache: only store executable permission bit #11541
Conversation
|
I've merged #11534 so you can rebase now :-) |
2be4ff3 to
359a4ed
Compare
|
Done! |
| { st_kind = stats.st_kind; st_perm = stats.st_perm } | ||
| (* Check if any of the +x bits are set, ignore read and write *) | ||
| let executable = 0o111 land stats.st_perm <> 0 in | ||
| { st_kind = stats.st_kind; executable } |
There was a problem hiding this comment.
I kinda wonder whether Path.Permissions.executable would make sense to be used here, but maybe it is not worth the hassle.
There was a problem hiding this comment.
Path.Permissions.(test executable) will only test if the flag is set for the current user (u+x, or 0o100)
I wanted here to explicitly test if that flag is set for anyone.
Granted, files with [u-x g+x o+x] are probably few and far between, but still.
|
Is there a discussion of this design change anywhere? I'm curious for the motivation. |
|
There is only the discussion happening in the issue #11533. My motivation was fixing the issue :) |
|
I'm curious: do we know the effect it will have on existing cache codebase? Will it retrigger the computation for the already cached elements? |
I would assume so since the change is in |
|
Note that we can't just change the behavior and call it a fix. This is observable from build rules. Concretely, if you have a rule like this: The That "simplified" mode like git uses might be beneficial, but it can be surprising, and there might be build rules that depend on the full permissions. |
|
In such case storing and restoring the exact permissions should be 100% backwards compatible, while also solving the issue. Is it worth adjusting the cache representation to allow to store the permissions? I don't know, it could be also some change that will not make a difference in real life. @rgrinberg What's your take on this? |
I'm working on a PR to do just that, as I think the naive approach of this PR may be too naive |
|
Seems fine to me. CHANGES entry please. |
|
I've added a CHANGES entry. I tried doing what is suggested above, you can see my progress [here].(https://github.com/ocaml/dune/compare/main...ElectreAAS:dune:digest-full-perm?expand=1)
edit: I feel like I'm going somewhere with my other approach, stay tuned |
|
I'm not sure restoring the full permissions is desirable, as I explained in #11533 . Most notably, if dune cache still removes write permissions in hardlink mode the interaction will be weird. Using I don't think "group" and "other" permissions belong in a cache or a versioning system since they are local to each machine, I'm not sure it's worth dealing with the weird |
56b5aed to
746d4ae
Compare
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
746d4ae to
4140e38
Compare
Oh sorry I missed your message there. Even if it still feels wrong to change the behaviour, as pointed by emillon, I think you've convinced me, so let's go with this approach. |
CHANGES: ### Fixed - Support HaikuOS: don't call `execve` since it's not allowed if other pthreads have been created. The fact that Haiku can't call `execve` from other threads than the principal thread of a process (a team in haiku jargon), is a discrepancy to POSIX and hence there is a [bug about it](https://dev.haiku-os.org/ticket/18665). (@Sylvain78, ocaml/dune#10953) - Fix flag ordering in generated Merlin configurations (ocaml/dune#11503, @voodoos, fixes ocaml/merlin#1900, reported by @vouillon) ### Added - Add `(format-dune-file <src> <dst>)` action. It provides a replacement to `dune format-dune-file` command. (ocaml/dune#11166, @nojb) - Allow the `--prefix` flag when configuring dune with `ocaml configure.ml`. This allows to set the prefix just like `$ dune install --prefix`. (ocaml/dune#11172, @rgrinberg) - Allow arguments starting with `+` in preprocessing definitions (starting with `(lang dune 3.18)`). (@amonteiro, ocaml/dune#11234) - Support for opam `(maintenance_intent ...)` in dune-project (ocaml/dune#11274, @art-w) - Validate opam `maintenance_intent` (ocaml/dune#11308, @art-w) - Support `not` in package dependencies constraints (ocaml/dune#11404, @art-w, reported by @hannesm) ### Changed - Warn when failing to discover root due to reads failing. The previous behavior was to abort. (@KoviRobi, ocaml/dune#11173) - Use shorter path for inline-tests artifacts. (@hhugo, ocaml/dune#11307) - Allow dash in `dune init` project name (ocaml/dune#11402, @art-w, reported by @saroupille) - On Windows, under heavy load, file delete operations can sometimes fail due to AV programs, etc. Guard against it by retrying the operation up to 30x with a 1s waiting gap (ocaml/dune#11437, fixes ocaml/dune#11425, @MSoegtropIMC) - Cache: we now only store the executable permission bit for files (ocaml/dune#11541, fixes ocaml/dune#11533, @ElectreAAS) - Display negative error codes on Windows in hex which is the more customary way to display `NTSTATUS` codes (ocaml/dune#11504, @MisterDA)
CHANGES: ### Fixed - Support HaikuOS: don't call `execve` since it's not allowed if other pthreads have been created. The fact that Haiku can't call `execve` from other threads than the principal thread of a process (a team in haiku jargon), is a discrepancy to POSIX and hence there is a [bug about it](https://dev.haiku-os.org/ticket/18665). (@Sylvain78, ocaml/dune#10953) - Fix flag ordering in generated Merlin configurations (ocaml/dune#11503, @voodoos, fixes ocaml/merlin#1900, reported by @vouillon) ### Added - Add `(format-dune-file <src> <dst>)` action. It provides a replacement to `dune format-dune-file` command. (ocaml/dune#11166, @nojb) - Allow the `--prefix` flag when configuring dune with `ocaml configure.ml`. This allows to set the prefix just like `$ dune install --prefix`. (ocaml/dune#11172, @rgrinberg) - Allow arguments starting with `+` in preprocessing definitions (starting with `(lang dune 3.18)`). (@amonteiro, ocaml/dune#11234) - Support for opam `(maintenance_intent ...)` in dune-project (ocaml/dune#11274, @art-w) - Validate opam `maintenance_intent` (ocaml/dune#11308, @art-w) - Support `not` in package dependencies constraints (ocaml/dune#11404, @art-w, reported by @hannesm) ### Changed - Warn when failing to discover root due to reads failing. The previous behavior was to abort. (@KoviRobi, ocaml/dune#11173) - Use shorter path for inline-tests artifacts. (@hhugo, ocaml/dune#11307) - Allow dash in `dune init` project name (ocaml/dune#11402, @art-w, reported by @saroupille) - On Windows, under heavy load, file delete operations can sometimes fail due to AV programs, etc. Guard against it by retrying the operation up to 30x with a 1s waiting gap (ocaml/dune#11437, fixes ocaml/dune#11425, @MSoegtropIMC) - Cache: we now only store the executable permission bit for files (ocaml/dune#11541, fixes ocaml/dune#11533, @ElectreAAS) - Display negative error codes on Windows in hex which is the more customary way to display `NTSTATUS` codes (ocaml/dune#11504, @MisterDA)
This PR is built on top of
the not-yet-merged #11534main!, and includes the fix proposed in #11533.We just stop storing the read & write permissions bits, only store the executable one. The strategy is the same as in git, we check if any of the u+x/g+x/o+x bits are set.