ocamldep: add -strict (error if input doesnt exist) by emillon · Pull Request #11593 · ocaml/ocaml

emillon · 2022-10-03T11:14:05Z

When a nonexistent input file is passed to ocamldep, it is ignored and the error is not reported. This PR adds a -strict flag that prevents this behavior.

Consider the following situation:

on one side, ocamldep is being invoked as ocamldep -modules a.ml
on the other side, a.ml is unlinked and created again.

(this corresponds to what happens when dune is running and hg or git is updating its tree in the meantime - updates are not always atomic if hardlinks are involved for example)

We want to make sure that when ocamldep exits with 0, its output corresponds to either the old or the new version of the file. This is enabled by -strict.

emillon · 2022-10-03T11:15:17Z

(as for the PR itself, I tried to add some tests to cover this but let me know if there's a better way to do this or if this should be expanded to cover more cases - in our use case, -modules is the thing that matters.)

emillon · 2022-10-03T11:15:42Z

cc @snowleopard

nojb

LGTM generally; the manual needs to be updated though.

Changes

driver/makedepend.ml

emillon · 2022-10-03T13:19:57Z

@nojb thanks. I amended ocamldep.1 and ocamldep.etex (and also reworded to "exit with nonzero ~~exit~~ code")

shindere · 2022-10-03T13:54:19Z

Grmbl, it seems my comment didn't make it to the PR. Etienne Millon (2022/10/03 06:20 -0700):

@nojb thanks. I amended `ocamldep.1` and `ocamldep.etex` (and also reworded to "exit with nonzero ~exit~ code")

How about "fail"?

shindere · 2022-10-03T13:56:30Z

I was also suggesting in the lost comment to keep the options in almost-alphabetic order and to define `-strict` just after `-sort`

shindere · 2022-10-03T13:57:42Z

Finally, regarding the testsuite: how about gathering all the tests in one file? It seems to me it would make the tests easier to follow with an overall picture of what is being tested and ther coudl be some factorization.

nojb · 2022-10-03T15:18:33Z

testsuite/tests/tool-ocamldep-strict/strict_modules_error.ml

@@ -0,0 +1,8 @@
+(* TEST
+modules = "dep.ml a.ml"


I think it is not necessary to test both with, and without -modules, since the behaviour being changed is orthogonal to this flag. I would leave only the one without -modules and remove the one with -modules.

I removed the ones with -modules. I'd like to implement @shindere 's suggestion to group things in a single test, but I'm not sure to do it. This is my first time with ocamltest. How can I specify that the same test should be run with different options and different expected outcomes?

nojb · 2022-10-03T15:20:59Z

testsuite/tests/tool-ocamldep-strict/classic_error.ml

+* setup-ocamlc.byte-build-env
+** ocamlc.byte
+commandline = "-depend a.ml doesnotexist.ml"
+*** check-ocamlc.byte-output


I'm not sure we want to test the output of ocamldep in this test; the feature being added seems unrelated. Perhaps let's simplify the patch by removing this test step and the "reference" file?

I agree. There's one gotcha in the current implementation though: if we pass as input A B and C, and B does not exist, the output will still contain dependency information for A and C (but with a nonzero exit code). That test kind of characterizes that behaviour so that's why I included it. But yes it can be argued that the output of a failing command is not to be relied upon, so I'm fine with removing the check on the output.

Changes

driver/makedepend.ml

shindere · 2022-10-03T16:27:09Z

I fully agree with @nojb's comments Nicolás Ojeda Bär (2022/10/03 08:21 -0700):

```suggestion " If an input file does not exist, exit with a nonzero code" ```

I tried to suggest " Fail ff an input file does not exist". To me, it feels that in the context of a Unix program, "fail" should be clear enough to mean return non-zero...

shindere · 2022-10-09T18:16:50Z

Etienne Millon (2022/10/07 02:24 -0700):

@emillon commented on this pull request. > @@ -0,0 +1,7 @@ +(* TEST +modules = "dep.ml a.ml" +* setup-ocamlc.byte-build-env +** ocamlc.byte +commandline = "-depend a.ml doesnotexist.ml" +*** check-ocamlc.byte-output I agree. There's one gotcha in the current implementation though: if we pass as input A B and C, and B does not exist, the output will still contain dependency information for A and C (but with a nonzero exit code). That test kind of characterizes that behaviour so that's why I included it. But yes it can be argued that the output of a failing command is not to be relied upon, so I'm fine with removing the check on the output.

Do you think it would be a lot of work to test for all the file's existence before even starting to output the dependencies? That way you could guarantee that nothing is printed if in strict mode and at least one file does not exist. You could even print the list of non-existent files on stderr in that case, if you like the idea.

shindere · 2022-10-09T18:19:46Z

Etienne Millon (2022/10/07 02:18 -0700):

@emillon commented on this pull request. > @@ -0,0 +1,8 @@ +(* TEST +modules = "dep.ml a.ml" I removed the ones with -modules. I'd like to implement @shindere 's suggestion to group things in a single test, but I'm not sure to do it. This is my first time with ocamltest. How can I specify that the same test should be run with different options and different expected outcomes?

See e.g. `testsuite/tests/asmcomp/func_sections.ml` for an example. If you implement the other suggestion you could add meaningful tests even in the case where `-strict` is specified and one or more files are missing.

shindere · 2022-10-11T07:21:26Z

Nicolás Ojeda Bär (2022/10/03 05:11 -0700):

Is it a good idea to specify the exact exit code? As far as I can see, `we exit with 2 whenever *any* error occurs, so it may be better to use a more general wording: ```suggestion " if an input file does not exist, exit with a nonzero exit code" ```

How about ``` " fail if an input file does not exist" ``` ? Also, nitpick but, the options are roughly sorted in alphabetical order, so how about moving the definition of `-strict` just under the one of `-sort`? Regarding the testsuite: wouldn't it make things more readable to gather the tests in the same file? I think they all share the same root test and the same definition of the `modules` variable, so it feels to me it would make it easier to get an overall view of what is being tested. What do you think?

mshinwell · 2022-10-13T13:58:19Z

@emillon I wonder if you might consider a slightly different fix, as per this PR: oxcaml/oxcaml#892

This removes the file-exists test entirely in strict mode, relying on the existing exception handler around the "open" call (which might be in a ppx subprocess). This should avoid a race where the file is deleted between the file-exists test and the open.

nojb · 2022-12-30T04:15:14Z

Friendly ping.

@mshinwell made a suggestion about a different implementation which sounds slightly superior (as it avoids a race condition). @emillon: do you think you could give it a shot?

nojb · 2023-02-23T14:26:32Z

Friendly ping. @emillon: are you still interested in working on this issue?

snowleopard · 2023-02-24T07:38:39Z

I'd like to add that at Jane Street we've been happily using @mshinwell's patch in our build system for a while.

emillon · 2023-02-24T08:55:07Z

Sorry for the long silence. I wasn't sure what was the situation at JS. I'll update this with @mshinwell's patch.

nojb · 2023-02-24T08:56:22Z

Sorry for the long silence. I wasn't sure what was the situation at JS. I'll update this with @mshinwell's patch.

Thanks!

nojb · 2023-03-11T11:54:44Z

Friendly ping. Any news?

emillon · 2023-03-13T14:21:36Z

I updated the PR and the test suite output. Let me know what you think. Sorry for leaving that aside.

Closes ocaml#8625 See oxcaml/oxcaml#892 When a nonexistent input file is passed to ocamldep, it is ignored and the error is not reported. This PR adds a `-strict` flag that prevents this behavior. Consider the following situation: - on one side, ocamldep is being invoked as `ocamldep -modules a.ml` - on the other side, `a.ml` is unlinked and created again. (this corresponds to what happens when `dune` is running and `hg` or `git` is updating its tree in the meantime - updates are not always atomic if hardlinks are involved for example) We want to make sure that when ocamldep exits with 0, its output corresponds to either the old or the new version of the file. This is enabled by `-strict`.

nojb

Looks generally good. I'm only wondering about the exact semantics of the new flag. Given the placement of the test in a try catch clause, it looks like "strict" mode will apply to any error during the processing of the file (eg syntax error), not just in case the file does not exist.

This seems fine to me; but if that's the case, I would like the documentation appearing in the help text of the tool and the manual/man page to be adapted accordingly.

nojb · 2023-03-18T07:37:22Z

driver/makedepend.ml

  with x -> begin
    print_exception x;
-    if not !allow_approximation then begin
+    if !strict || not !allow_approximation then begin


Do I understand correctly that in -strict mode the tool will exit with a non-zero code also if the file contains a syntax error or some other is raised when processing it (not just if the file does not exist)? If yes, the documentation should be adapted accordingly.

nojb · 2025-05-14T14:17:57Z

PR open for two years without any activity. As assignee, I am moving to close it; to be reopened as needed.

nojb reviewed Oct 3, 2022

View reviewed changes

Changes Outdated Show resolved Hide resolved

driver/makedepend.ml Outdated Show resolved Hide resolved

nojb reviewed Oct 3, 2022

View reviewed changes

emillon force-pushed the ocamldep-strict branch 2 times, most recently from cf185c2 to a0b1c62 Compare October 7, 2022 09:16

mshinwell mentioned this pull request Oct 13, 2022

Add strict mode for ocamldep oxcaml/oxcaml#892

Merged

nojb added tools ocamldep labels Dec 30, 2022

nojb self-assigned this Feb 23, 2023

emillon force-pushed the ocamldep-strict branch from a0b1c62 to e5e3709 Compare March 13, 2023 14:20

emillon force-pushed the ocamldep-strict branch from e5e3709 to 05c0e0d Compare March 13, 2023 14:21

nojb reviewed Mar 18, 2023

View reviewed changes

nojb closed this May 14, 2025

Conversation

emillon commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emillon commented Oct 3, 2022

Uh oh!

emillon commented Oct 3, 2022

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emillon commented Oct 3, 2022

Uh oh!

shindere commented Oct 3, 2022 via email

Uh oh!

shindere commented Oct 3, 2022 via email

Uh oh!

shindere commented Oct 3, 2022 via email

Uh oh!

nojb Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

emillon Oct 7, 2022

Choose a reason for hiding this comment

Uh oh!

nojb Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

emillon Oct 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shindere commented Oct 3, 2022 via email

Uh oh!

shindere commented Oct 9, 2022 via email

Uh oh!

shindere commented Oct 9, 2022 via email

Uh oh!

shindere commented Oct 11, 2022 via email

Uh oh!

mshinwell commented Oct 13, 2022

Uh oh!

nojb commented Dec 30, 2022

Uh oh!

nojb commented Feb 23, 2023

Uh oh!

snowleopard commented Feb 24, 2023

Uh oh!

emillon commented Feb 24, 2023

Uh oh!

nojb commented Feb 24, 2023

Uh oh!

nojb commented Mar 11, 2023

Uh oh!

emillon commented Mar 13, 2023

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

nojb Mar 18, 2023

Choose a reason for hiding this comment

Uh oh!

nojb commented May 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emillon commented Oct 3, 2022 •

edited

Loading