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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions otherlibs/stdune/src/fpath.ml
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,9 @@ let traverse =
let traverse_files ~dir ~init ~f =
traverse ~dir ~init ~on_dir:(fun ~dir:_ _fname acc -> acc) ~on_file:f
;;

let is_rw p =
match Unix.access p Unix.[ R_OK; W_OK ] with
| () -> true
| exception Unix.Unix_error _ -> false
;;
3 changes: 3 additions & 0 deletions otherlibs/stdune/src/fpath.mli
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,6 @@ val traverse_files
-> init:'acc
-> f:(dir:string -> Filename.t -> 'acc -> 'acc)
-> 'acc

(** Is readable and writable. *)
val is_rw : string -> bool
1 change: 1 addition & 0 deletions otherlibs/stdune/src/path.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,7 @@ module Source = struct

let is_in_build_dir s = is_in_build_dir (path_of_local s)
let to_local t = t
let is_rw t = Fpath.is_rw (to_string t)
end

let set_of_source_paths set = Source.Set.to_list set |> Set.of_list_map ~f:source
Expand Down
3 changes: 3 additions & 0 deletions otherlibs/stdune/src/path.mli
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ module Source : sig
val descendant : t -> of_:t -> t option
val to_local : t -> Local.t

(** Whether the file at the given path is readable and writable. *)
val is_rw : t -> bool

module Table : Hashtbl.S with type key = t
end

Expand Down
3 changes: 3 additions & 0 deletions src/dune_rules/format_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ let gen_rules_output
let* dialect, kind =
Path.Source.extension file |> Dialect.DB.find_by_extension dialects
in
(* Avoid setting up rules that will fail during promotion due to the
source file not being writable. *)
let* () = Option.some_if (Path.Source.is_rw file) () in
let* () =
Option.some_if (Format_config.includes config (Dialect (Dialect.name dialect))) ()
in
Expand Down
2 changes: 2 additions & 0 deletions test/blackbox-tests/test-cases/dialects/good.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?


$ dune build @fmt
Formatting main.mf
Formatting main.mfi
2 changes: 2 additions & 0 deletions test/blackbox-tests/test-cases/dialects/no_impl.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Test the (dialect ...) stanza inside the `dune-project` file.

$ dune exec ./main.exe

$ chmod a+w *

$ dune build @fmt
fake ocamlformat is running: "--impl" "fmt.ml"
fake ocamlformat is running: "--impl" "main.ml"
Expand Down
2 changes: 2 additions & 0 deletions test/blackbox-tests/test-cases/dialects/no_intf_good.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ Test the (dialect ...) stanza inside the dune-project file.

$ dune exec ./main.exe

$ chmod a+w *

$ dune build @fmt
fake ocamlformat is running: "--impl" "fmt.ml"
Formatting main.mf
Expand Down
4 changes: 4 additions & 0 deletions test/blackbox-tests/test-cases/formatting/feature.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -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.


Formatting can be checked using the @fmt target:

$ touch .ocamlformat
Expand Down
13 changes: 1 addition & 12 deletions test/blackbox-tests/test-cases/read-only-symlink-target.t/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,9 @@ Nix can leave a symlink to a store path in the tree, often called 'result'.
$ chmod -R a-w "$RESULT"
$ ln -s "$RESULT" result

This command should succeed:
Formatting 'foo.ml' shouldn't be attempted and this command should succeed:

$ dune fmt
File "ocamlformat.ml", line 1, characters 0-0:
Error: Files _build/default/ocamlformat.ml and
_build/default/.formatted/ocamlformat.ml differ.
File "result/foo.ml", line 1, characters 0-0:
Error: Files _build/default/result/foo.ml and
_build/default/result/.formatted/foo.ml differ.
Promoting _build/default/.formatted/ocamlformat.ml to ocamlformat.ml.
Promoting _build/default/result/.formatted/foo.ml to result/foo.ml.
Error: failed to promote result/foo.ml
Permission denied
[1]

Allow Dune to remove temporary files (calling Dune crashes without this):

Expand Down
Loading