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

Fix unreadable source dirs #2004

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Conversation

rgrinberg
Copy link
Member

This PR makes it so that unreadable directories are treated as empty. Any time an unreadable dir is encountered, we report this as a warning.

One thing that would have been nicer would be to completely exclude them from the contents, but that breaks laziness a bit. Because we have to try reading the dir before adding it to the map.

@rgrinberg rgrinberg requested a review from a user April 2, 2019 08:01
Copy link
Collaborator

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

Commenting to remove my review request [which seems spurious]

@rgrinberg
Copy link
Member Author

I also made our symlink check warn instead of error. That seems to be a bit more friendly.

@rgrinberg
Copy link
Member Author

I've reverted to the old behavior regarding symlinks. It's not working in CI for some reason.

@rgrinberg
Copy link
Member Author

@diml this is ready by the way

@rgrinberg
Copy link
Member Author

One concrete situation where the extra laziness helped was to avoid sorting directories when they were all ignored. That seems like an edge case though.

Now that we're only catching Unix errors, I've changed the result type to (_, Unix.error) Result.t. Using Or_exn.t is confusing since the function still raises in some situations (if Path.stat raises for example).

@rgrinberg
Copy link
Member Author

@emillon or @aalekseyev, mind taking over the review?

@rgrinberg rgrinberg force-pushed the file-tree-perms branch 2 times, most recently from 48fbbbd to b446fd6 Compare April 8, 2019 12:43
@rgrinberg
Copy link
Member Author

I'll add a test here by the way.

@rgrinberg
Copy link
Member Author

By the way, this is the last PR that I'd like to get in for 1.9

@ghost
Copy link

ghost commented Apr 8, 2019

Ok, I had another look. This looks good, but why did you make the loading of dune-project files lazy? In practice, it means that errors in dune-project files might not be reported as eagerly as before which is not great.

Apart from this, it looks good to me.

@rgrinberg rgrinberg force-pushed the file-tree-perms branch 2 times, most recently from 6ac5efb to 3651804 Compare April 9, 2019 08:27
@rgrinberg
Copy link
Member Author

@diml I removed the laziness and I've also added another feature:

If the directory is not readable, it will not be added as a node to File_tree.t. This costs us some laziness, but I think it's quite error prone to include such nodes in File_tree and have them be accessible via fold for example.

@rgrinberg rgrinberg force-pushed the file-tree-perms branch 2 times, most recently from 5f17026 to a2b62d4 Compare April 9, 2019 08:32
@ghost
Copy link

ghost commented Apr 9, 2019

Ok, that seems good

Any node that fails to be read will now just emit a warning and be otherwise
ignored in the build.

Signed-off-by: Rudi Grinberg <[email protected]>
@rgrinberg
Copy link
Member Author

@diml your last change makes it so that Dune_project.anonymous is always forced. Not a big deal, but it's the reason why I kept that extra bit of laziness.

@rgrinberg rgrinberg merged commit 7388c24 into ocaml:master Apr 9, 2019
@rgrinberg rgrinberg deleted the file-tree-perms branch April 9, 2019 09:05
@ghost
Copy link

ghost commented Apr 9, 2019

Ok. Yh it doesn't matter in practice, the only reason anonymous is lazy is because it needs to be evaluated after the "dune" language is registered. Computing it is effectively fast and not worth avoiding.

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 9, 2019
CHANGES:

- Warn when generated `.merlin` does not reflect the preprocessing
  specification. This occurs when multiple stanzas in the same directory use
  different preprocessing specifications. This warning can now be disabled with
  `allow_approx_merlin` (ocaml/dune#1947, fix ocaml/dune#1946, @rgrinberg)

- Watch mode: display "Success" in green and "Had errors" in red (ocaml/dune#1956,
  @emillon)

- Fix glob dependencies on installed directories (ocaml/dune#1965, @rgrinberg)

- Add support for library variants and default implementations. (ocaml/dune#1900,
  @TheLortex)

- Add experimental `$ dune init` command. This command is used to create or
  update project boilerplate. (ocaml/dune#1448, fixes ocaml/dune#159, @shonfeder)

- Experimental Coq support (fix ocaml/dune#1466, @ejgallego)

- Install .cmi files of private modules in a `.private` directory (ocaml/dune#1983, fix
  ocaml/dune#1973 @rgrinberg)

- Fix `dune subst` attempting to substitute on directories. (ocaml/dune#2000, fix ocaml/dune#1997,
  @rgrinberg)

- Do not list private modules in the generated index. (ocaml/dune#2009, fix ocaml/dune#2008,
  @rgrinberg)

- Warn instead of failing if an opam file fails to parse. This opam file can
  still be used to define scope. (ocaml/dune#2023, @rgrinberg)

- Do not crash if unable to read a directory when traversing to find root
  (ocaml/dune#2024, @rgrinberg)

- Do not exit dune if some source directories are unreadable. Instead, warn the
  user that such directories need to be ignored (ocaml/dune#2004, fix ocaml/dune#310, @rgrinberg)

- Fix nested `(binaries ..)` fields in the `env` stanza. Previously, parent
  `binaries` fields would be ignored, but instead they should be combined.
  (ocaml/dune#2029, @rgrinberg)

- Allow "." in `c_names` and `cxx_names` (ocaml/dune#2036, fix ocaml/dune#2033, @rgrinberg)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 9, 2019
CHANGES:

- Warn when generated `.merlin` does not reflect the preprocessing
  specification. This occurs when multiple stanzas in the same directory use
  different preprocessing specifications. This warning can now be disabled with
  `allow_approx_merlin` (ocaml/dune#1947, fix ocaml/dune#1946, @rgrinberg)

- Watch mode: display "Success" in green and "Had errors" in red (ocaml/dune#1956,
  @emillon)

- Fix glob dependencies on installed directories (ocaml/dune#1965, @rgrinberg)

- Add support for library variants and default implementations. (ocaml/dune#1900,
  @TheLortex)

- Add experimental `$ dune init` command. This command is used to create or
  update project boilerplate. (ocaml/dune#1448, fixes ocaml/dune#159, @shonfeder)

- Experimental Coq support (fix ocaml/dune#1466, @ejgallego)

- Install .cmi files of private modules in a `.private` directory (ocaml/dune#1983, fix
  ocaml/dune#1973 @rgrinberg)

- Fix `dune subst` attempting to substitute on directories. (ocaml/dune#2000, fix ocaml/dune#1997,
  @rgrinberg)

- Do not list private modules in the generated index. (ocaml/dune#2009, fix ocaml/dune#2008,
  @rgrinberg)

- Warn instead of failing if an opam file fails to parse. This opam file can
  still be used to define scope. (ocaml/dune#2023, @rgrinberg)

- Do not crash if unable to read a directory when traversing to find root
  (ocaml/dune#2024, @rgrinberg)

- Do not exit dune if some source directories are unreadable. Instead, warn the
  user that such directories need to be ignored (ocaml/dune#2004, fix ocaml/dune#310, @rgrinberg)

- Fix nested `(binaries ..)` fields in the `env` stanza. Previously, parent
  `binaries` fields would be ignored, but instead they should be combined.
  (ocaml/dune#2029, @rgrinberg)

- Allow "." in `c_names` and `cxx_names` (ocaml/dune#2036, fix ocaml/dune#2033, @rgrinberg)

- Format rules: if a dune file uses OCaml syntax, do not format it.
  (ocaml/dune#2014, fix ocaml/dune#2012, @emillon)
@rgrinberg rgrinberg restored the file-tree-perms branch April 20, 2019 05:46
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