Cache: only store executable permission bit by ElectreAAS · Pull Request #11541 · ocaml/dune

ElectreAAS · 2025-03-17T14:59:02Z

This PR is built on top of ~~the not-yet-merged #11534~~ main!, 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.

Leonidas-from-XIV · 2025-03-18T15:33:06Z

I've merged #11534 so you can rebase now :-)

ElectreAAS · 2025-03-18T19:29:05Z

Done!

Leonidas-from-XIV · 2025-03-19T14:12:22Z

src/dune_digest/digest.ml

-    { 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 }


I kinda wonder whether Path.Permissions.executable would make sense to be used here, but maybe it is not worth the hassle.

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.

Alizter · 2025-03-19T14:45:22Z

Is there a discussion of this design change anywhere? I'm curious for the motivation.

ElectreAAS · 2025-03-19T15:55:28Z

There is only the discussion happening in the issue #11533. My motivation was fixing the issue :)
In a conversation with emillon we also linked https://archive.kernel.org/oldwiki/git.wiki.kernel.org/index.php/ContentLimitations.html for the info of "git only stores the +x flag"

maiste · 2025-03-20T08:36:36Z

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?

Leonidas-from-XIV · 2025-03-20T11:28:51Z

Will it retrigger the computation for the already cached elements?

I would assume so since the change is in Stats_for_digest and that's probably the digest the cache uses to look up files. Which is.. ok I believe, if this is the way we think it should be done.

emillon · 2025-03-21T08:14:43Z

Note that we can't just change the behavior and call it a fix. This is observable from build rules.
For example chmodding a file would cause a rebuild of anything that depends on it, and it wouldn't anymore.

Concretely, if you have a rule like this:

(rule
 (with-stdout-to stat.txt
 (run stat file.txt)))

The stat.txt in _build won't be updated when the permissions change on file.txt.

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.

Leonidas-from-XIV · 2025-03-21T11:09:34Z

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?

ElectreAAS · 2025-03-21T18:03:42Z

In such case storing and restoring the exact permissions should be 100% backwards compatible, while also solving the issue.

I'm working on a PR to do just that, as I think the naive approach of this PR may be too naive

rgrinberg · 2025-03-22T11:55:13Z

Seems fine to me. CHANGES entry please.

ElectreAAS · 2025-03-24T18:37:48Z

I've added a CHANGES entry.
I am however still unconvinced by this approach, it feels like a cop out.

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)

but I've ran into a problem: storing the permissions of files means we need to compute (stat) them somewhere, and then carry them to where the store happens. This has meant changing Digest.t Targets.Produced.t into (Digest.t * Unix.file_perm) Targets.Produced.t in a million different places, which feels like I'm taking the wrong approach...

edit: I feel like I'm going somewhere with my other approach, stay tuned

mefyl · 2025-03-25T07:43:41Z

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 stat (2), one can also read the number of hardlink references to a file, and that definitely cannot be "restored". This, to my, implies that some file metadata is to be considered out of the scope of dune and that having rules depend on it is unreasonable.

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 u-r situation since then dune will not even be able to hash the file and things might not work anyway, and I think the dune cache will force u-w in hardlink mode anyway. I could be misguided, but I think it's worth double checking we actually want to restore these permissions before investing work in it.

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS · 2025-03-25T12:16:47Z

I'm not sure restoring the full permissions is desirable, as I explained in #11533.

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.
I've rebased on main, and added the changelog entry, and people seem to generally approve - I'll merge once CI is green

@Sylvain78

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)

@Sylvain78

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)

ElectreAAS requested a review from emillon March 17, 2025 14:59

maiste added the shared-cache Shared artefacts cache label Mar 17, 2025

ElectreAAS force-pushed the digest-no-perm branch from 2be4ff3 to 359a4ed Compare March 18, 2025 19:28

Leonidas-from-XIV reviewed Mar 19, 2025

View reviewed changes

Alizter requested a review from rgrinberg March 19, 2025 16:16

Leonidas-from-XIV mentioned this pull request Mar 21, 2025

cache: some permissions can't be represented, causing cache misses #11533

Closed

rgrinberg approved these changes Mar 22, 2025

View reviewed changes

ElectreAAS force-pushed the digest-no-perm branch from 56b5aed to 746d4ae Compare March 25, 2025 12:11

ElectreAAS added 2 commits March 25, 2025 13:13

Remove read-write permissions in stats_for_digests

602edab

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Added changelog entry

4140e38

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS force-pushed the digest-no-perm branch from 746d4ae to 4140e38 Compare March 25, 2025 12:13

ElectreAAS merged commit 0442b11 into main Mar 25, 2025
24 of 25 checks passed

ElectreAAS deleted the digest-no-perm branch March 25, 2025 16:33

maiste mentioned this pull request Mar 31, 2025

[new release] dune (15 packages) (3.18.0~alpha0) ocaml/opam-repository#27696

Closed

maiste mentioned this pull request Apr 3, 2025

[new release] dune (15 packages) (3.18.0) ocaml/opam-repository#27704

Merged

Comments

Conversation

ElectreAAS commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leonidas-from-XIV commented Mar 18, 2025

Uh oh!

ElectreAAS commented Mar 18, 2025

Uh oh!

Leonidas-from-XIV Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

ElectreAAS Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Alizter commented Mar 19, 2025

Uh oh!

ElectreAAS commented Mar 19, 2025

Uh oh!

maiste commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leonidas-from-XIV commented Mar 20, 2025

Uh oh!

emillon commented Mar 21, 2025

Uh oh!

Leonidas-from-XIV commented Mar 21, 2025

Uh oh!

ElectreAAS commented Mar 21, 2025

Uh oh!

rgrinberg commented Mar 22, 2025

Uh oh!

ElectreAAS commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mefyl commented Mar 25, 2025

Uh oh!

ElectreAAS commented Mar 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ElectreAAS commented Mar 17, 2025 •

edited

Loading

maiste commented Mar 20, 2025 •

edited

Loading

ElectreAAS commented Mar 24, 2025 •

edited

Loading