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

Avoid formatting read-only files #11490

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Julow
Copy link
Contributor

@Julow Julow commented Feb 19, 2025

This follows the discussion in #11202

Formatting a read-only file might succeed but the promotion step will fail and disturb the workflow. This checks each source file access before setting up the formatting rule.

This happens often with Nix's 'result' links, which might contain .ml files when an OCaml project was built.
Ideally, Dune wouldn't look inside 'result' links as they often contain outdated installables for the local packages but changing that might interfere with symlinks to other dune workspaces.

Formatting a read-only file might succeed but the promotion step will
fail and disturb the workflow. This checks each source file access
before setting up the formatting rule.

This happens often with Nix's 'result' links, which might contain .ml
files when an OCaml project was built.
Ideally, Dune wouldn't look inside 'result' links as they often contain
outdated installables for the local packages but changing that might
interfere with symlinks to other dune workspaces.
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Julow You need to sign off these commits to make the DCO check happy :)

(The CI failure about Dynlink failing is unrelated)

@@ -2,6 +2,8 @@ Test the (dialect ...) stanza inside the dune-project file.

$ dune exec ./main.exe

$ chmod a+w *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment on these chmods why this is necessary? Someone unaware of that functionality will probably wonder what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the same reason as #11490 (comment) in feature.t. I wonder if there's a better way to do it ?

@@ -3,6 +3,10 @@ Note about versioning:
- in (lang dune 1.x), no formatting rules are set up by default.
- (lang dune 2.0) behaves as if (using fmt 1.2) is set.

Turn every symlinks into writable files belonging to the sandbox as formatting
rules are not generated for read-only files:
$ find . -type l | while read f; do (rm "$f"; cat > $f) < $f; done
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be easier with find -exec?

(OTOH fairly impressed it even works, reading the file and replacing it in one step)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the files (but not the directories) in the cram test's current directory are symlinks to read-only copies of the source code. This breaks dune promote but also breaks any programs that don't do the "write to temp file and move it to its destination" strategy, as opening the file for truncating and writing (eg. with open_out_bin) will not override the permissions.

-exec will not work because this uses two commands. Reading continue to work after the file is removed, after all the stdin is still a reference to it so it can't be freed yet. cat > $f outputs to a new file and doesn't disturb the reading.

Co-authored-by: Marek Kubica <[email protected]>
Signed-off-by: Jules Aguillon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants