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

ocamldep: .ml in earlier include dirs should shadow .mli in later include dir #2221

Merged
merged 4 commits into from Apr 10, 2019

Conversation

@nojb
Copy link
Contributor

commented Jan 17, 2019

This PR addresses the following problem of ocamldep discovered while working on #2218.

Suppose that you have the following files:

- a.ml
- dir1/b.ml
- dir2/b.ml
- dir2/b.mli

and a.ml mentions B inside it. If you use ocamldep to compute dependencies:

ocamldep -I dir1 -I dir2 a.ml

you will get

a.cmo : dir2/b.cmi
a.cmx : dir2/b.cmx

Rather, one would expect to get dependencies in dir1 since that directory is passed first.

The reason for this behaviour is that ocamldep looks for .mli files first and only if that fails it looks for .ml files. Instead, this PR proposes to look for both .mli and .ml files at the same time.

The diff is really small modulo indentation; I suggest to read it ignoring whitespace.

@gasche

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

(Is it possible to have a test for this?)

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Yes, should be possible. I will give it a try.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

Added a test.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

This seems to be a simpler way to resolve module conflicts in ocamldep. Did you check on the opam repository that no one relies on the current mli-first arbitrage?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

No, I didn't and am not sure how to do it. How could I go about it?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

A good first-order approximation might be to check that this PR does not break any opam packages. Otherwise, it might be interesting to detect situation where the current and new conflict resolutions leads to a different module resolution?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

A good first-order approximation might be to check that this PR does not break any opam packages.

I am not super familiar with opam; do you know of a script or instructions somewhere explaining how one can build "all" packages?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

I am not sure which tool is the more adapted here. Maybe opamcheck?

@diml

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Note that both dune and ocamlbuild are using ocamldep -modules, which I believe is not affected by this PR. So the vast majority of opam packages shouldn't be affected by this PR.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

Good point. Plus, I think it is rather unusual to have multiple modules with the same name in usual OCaml projects. So I'm not sure it is worth going through the trouble of setting up some opam-wide testing.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

What is the algorithm used by the compiler itself? Whatever it is, ocamldep should try to match it. Installing and using opamcheck is rather involved, so I don't think it's warranted for this PR.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

BTW the proper link for opamcheck is https://github.com/damiendoligez/opamcheck, the JS version of the project is stale.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

The compiler uses the first matching cmi and cm(x)o in the list of included directories. The current choice of ocamldep is thus respected by the compiler only until the point where the first directory cm{i,x} files are built (and there are potential failing states where non-matching cmi and cmo files are selected).

Copy link
Contributor

left a comment

The code itself looks good to me, and the new inferred dependency should lead to more stable build for the few people concerned.

@nojb nojb force-pushed the nojb:fix_ocamldep_shadowing branch from 9d761ac to 9663d5b Apr 9, 2019
@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

The code itself looks good to me, and the new inferred dependency should lead to more stable build for the few people concerned.

Thanks for the approval!

I rebased the PR and will merge it based on @Octachron's approval unless someone shouts.

@nojb nojb force-pushed the nojb:fix_ocamldep_shadowing branch from 9663d5b to aebc58a Apr 10, 2019
@nojb nojb merged commit 3fac824 into ocaml:trunk Apr 10, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb nojb deleted the nojb:fix_ocamldep_shadowing branch Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.