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 one file per line #2189

Merged
merged 6 commits into from Dec 12, 2018

Conversation

Projects
None yet
5 participants
@gasche
Copy link
Member

commented Dec 8, 2018

I got tired of the noisy diffs introduced by running ocamldep in the middle of PRs that require it (adding a new file), etc. This PR implements a one-dependency-per-line option for ocamldep output, which should result in much shorter, neater ocamldep diffs.

@gasche gasche force-pushed the gasche:ocamldep-one-file-per-line branch from 7445538 to 3617fc1 Dec 8, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

Do we really need to preserve backwards-compatibility on this one? Can't we simply use the new format everytime?

@gasche

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2018

I have no opinion, but the cost of making this an option is extremely small.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

Technically, yes. But in my opinion usability is harmed by a proliferation of options, especially if there is no clear reason to use the "old" format.

@gasche gasche force-pushed the gasche:ocamldep-one-file-per-line branch from 5e1536d to b339fc6 Dec 9, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

It seems a nice idea, thanks!

Will the behaviour also work when the dependencies are computed
by calling ocamlc ?

@gasche: I guess parts of the diffs you are referring to are also due
to the dependencies of the C files. Any idea to normalize them, too?

@gasche

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2018

Yes, the behavior is shared between ocamldep and ocamlc -depend. This PR does not touch the behavior of {all,}depend for C files.

@gasche gasche force-pushed the gasche:ocamldep-one-file-per-line branch from b339fc6 to b4f7d49 Dec 10, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2018

@nojb I amended the PR to make the one-dep-per-file behavior non-optional. This will now affect all ocamldep output.

The previous behavior (with a command-line flag) is still available in a local branch ocamldep-one-file-per-line-option

@samoht

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Is that used at link time too? If not, you can you also sort the dependencies in lexicographical order to ensure some kind of stability.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

(Just to underline that the output mode triggered by
the -modules switch is not affected. Hence, build
systems relying on this mode are not affected.)

@gasche

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2018

@samoht I don't think we use this output for linking, but I would rather keep the dependency order unchanged for now (it may impact the Make build order in subtler way than I'd like). I will consider adding a sort later if I notice .depend noise caused by spurious changes in this order.

@gasche gasche force-pushed the gasche:ocamldep-one-file-per-line branch from b4f7d49 to 32447c8 Dec 11, 2018

@nojb

nojb approved these changes Dec 12, 2018

Copy link
Contributor

left a comment

LGTM.

@gasche gasche force-pushed the gasche:ocamldep-one-file-per-line branch from 32447c8 to c9d28a3 Dec 12, 2018

gasche added some commits Dec 8, 2018

Makefiles: restructure CAMLDEP usage to easily add flags
This change should be a refactoring no-op.

Before, a DEPFLAGS variable existed in some makefiles to contain
include directories to be passed to ocamldep invocations, but no
support for easily adding command-line flags to ocamldep was available
(invocations would systematically use -slash, which was duplicated
across callsites).

With this PR, a new DEPINCLUDES variable contains the include
directories, and DEPFLAGS is repurposed to contain other command-line
flags for the tool -- currently "slash".

@gasche gasche merged commit 49ff642 into ocaml:trunk Dec 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.