Skip to content

Test for issue #7454: fmt triggers menhir#7455

Merged
emillon merged 3 commits intoocaml:mainfrom
voodoos:issue-formatting-generate-menhir-parser
Apr 5, 2023
Merged

Test for issue #7454: fmt triggers menhir#7455
emillon merged 3 commits intoocaml:mainfrom
voodoos:issue-formatting-generate-menhir-parser

Conversation

@voodoos
Copy link
Copy Markdown
Collaborator

@voodoos voodoos commented Mar 30, 2023

See issue #7454.

Expected Behavior

I would expect dune fmt or dune build @fmt to never build anything.
(This is an assumption made by OCaml-CI for the lint-fmt job. cc @maiste)

Actual Behavior

Missing menhir parsers that have a corresponding .mli file are generated when calling dune build @fmt

Copy link
Copy Markdown
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

I guess the issue is that we're setting up formatting rules for all sources used for building. We should only be formatting sources that are present in the sources.

@anmonteiro
Copy link
Copy Markdown
Collaborator

I wonder if #6585 is related to this.

@rgrinberg
Copy link
Copy Markdown
Member

I wonder if #6585 is related to this.

Not quite. Dune files always exist in the source (at the moment)

Copy link
Copy Markdown
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

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

Sounds good to me too. Please add DCO and I'll rebase+merge.

Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
@voodoos voodoos force-pushed the issue-formatting-generate-menhir-parser branch from 29bca00 to c256209 Compare April 4, 2023 15:37
@voodoos
Copy link
Copy Markdown
Collaborator Author

voodoos commented Apr 4, 2023

Sounds good to me too. Please add DCO and I'll rebase+merge.

Done !

@emillon
Copy link
Copy Markdown
Collaborator

emillon commented Apr 4, 2023

Can you fix the test so that it uses dune_cmd exists instead of ls -a? That ensures that the output is consistent on mac and linux.

@rgrinberg
Copy link
Copy Markdown
Member

None of the uses in this PR require the -a flag either. So using ls without flags should be fine too

@emillon emillon merged commit e7ad170 into ocaml:main Apr 5, 2023
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.

4 participants