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

test: Repro 'dune fmt' crash in presence of Nix result #11202

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Julow
Copy link
Contributor

@Julow Julow commented Dec 13, 2024

Reproduction case for a crash when using dune fmt.
Perhaps formatting shouldn't follow symlinks ? Also, dune build doesn't seem to have any problem with that, perhaps because the linked directory doesn't contain a dune file.
Perhaps symlinked directories should be considered vendored_dirs automatically ?

@Julow Julow force-pushed the nix-result-fmt-crash branch from eeb7511 to a228ff9 Compare December 13, 2024 09:46
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

I have seen this before and it can be a bit puzzling.

IMO the issue here isn't that dune is following symlinks, but rather that the files in the nix store /nix/store/... have pretty locked-down permissions as one would expect.

I'm not saying that dune is perfect, just trying to point out the actual cause. There might be work to be done in dune itself.

@Julow
Copy link
Contributor Author

Julow commented Dec 13, 2024

Is here a case where we want the files in the symlinked directory be formatted ?

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.

Thanks for the repro case; I've posted some comments.

As to how to deal with projects like this, if we can't find the proper solution I would suggest adding it to the dev-meeting agenda to discuss.

test/blackbox-tests/test-cases/nix-result-in-tree.t/run.t Outdated Show resolved Hide resolved
test/blackbox-tests/test-cases/nix-result-in-tree.t/dune Outdated Show resolved Hide resolved
Execution will pass over me and through me. And when it has gone past, I
will unwind the stack along its path. Where the cases are handled there will
be nothing. Only I will remain.
[1]
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 filter the output a bit so it is more reproducible? The locations in the stacktrace are probably going to change over time. A simple grep EACCES is probably sufficient to show there's a very specific error that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hopted to remove the output and rely on the exit status.

test/blackbox-tests/test-cases/nix-result-in-tree.t/run.t Outdated Show resolved Hide resolved
@Julow Julow force-pushed the nix-result-fmt-crash branch from 9f4055b to be4fc2b Compare December 16, 2024 11:27
@maiste maiste added the nix label Dec 23, 2024
A 'ocamlformat' executable is added to the test to avoid depending on
OCamlformat.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants