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

pkg: fix dev-tool bug where dune fmt would revert file #10990

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Oct 4, 2024

The rule for running ocamlformat on source files when dev-tools are in use did not depend on the input file, so changes to the input file wouldn't cause ocamlformat to be re-run on the updated file. The consequence of this is that dune fmt would promote stale versions of files.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Oct 4, 2024

Fixes #10991

@moyodiallo
Copy link
Collaborator

moyodiallo commented Oct 4, 2024

Would you like add the test from #10993 ?

@rgrinberg
Copy link
Member

rgrinberg commented Oct 4, 2024

Fix looks good, but I think we need to improve things further to avoid mistakes like this. I would suggest the following steps:

  1. Demonstrate the bug in a test
  2. Make sure the formatting action is sandboxed when using package management
  3. Apply this fix

It might be easier to do 2. followed by 1.

EDIT: I see that the test has been created already. Let's take the opportunity to add sandboxing to demonstrate that this dependency is indeed missing.

@gridbugs
Copy link
Collaborator Author

gridbugs commented Oct 8, 2024

2. Make sure the formatting action is sandboxed when using package management

How would sandboxing the formatting action prevent mistakes like this?

@rgrinberg
Copy link
Member

If sandboxing was an enabled for the rule, the action would always fail with some error saying that it was unable to find the input file. Without sandboxing, the rule sometimes succeeds b/c the input was already in _build due to being needed for some other rule.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
The rule for running ocamlformat on source files when dev-tools are in
use did not depend on the input file, so changes to the input file
wouldn't cause ocamlformat to be re-run on the updated file. The
consequence of this is that `dune fmt` would promote stale versions of
files.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs
Copy link
Collaborator Author

If sandboxing was an enabled for the rule, the action would always fail with some error saying that it was unable to find the input file. Without sandboxing, the rule sometimes succeeds b/c the input was already in _build due to being needed for some other rule.

Ok I added sandboxing and confirmed that without the fix the formatting action can't find its input file.

@gridbugs gridbugs merged commit ed871ba into ocaml:main Oct 15, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants