Register more dependencies statically #667
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
|
Thanks |
ghost
changed the title
rgrinberg
force-pushed
the
more-static-deps
branch
from
2a991f9 to
996131c
Compare
|
I rebased this on top of master due to some blackbox test changes. Now every commit passes all the tests. |
rgrinberg
reviewed
Apr 3, 2018
| ~f:Build.return | ||
| in | ||
| let requires = | ||
| let with_lib_deps t compile_info ~dir ~has_dot_merlin ~f = |
There was a problem hiding this comment.
I think you might be forgetting to set ~has_dot_merlin correctly. I see it's being set to true everywhere rather than being based on the context.
added 9 commits
April 9, 2018 05:59
This way we know more of the compilation DAG statically. This helps for parallelism as well as for `jbuilder external-lib-deps`
rgrinberg
force-pushed
the
more-static-deps
branch
from
996131c to
f8b166c
Compare
|
I just ended up merging this to avoid keeping the PR up to date. Created an issue for the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most of the dependencies on files of library dependencies used to be declared dynamically. For instance the dependencies on
<lib-dir>/*.cmifor every library dependency. As a resultjbuilder external-lib-depswas quite often broken.This PR changes that by passing the
requiresvariable as a value of type(Lib.t list, exn) resultinstead of(unit, Lib.t list) Build.t. This usually simplify a bit the code as we have to go less through the build arrow. The dependency on the.merlinfile and theBuild.record_lib_depsthat used to be encoded inrequiresare now done withprefix_rules.One negative consequence of these changes is that we often have to pass two arguments instead of one to various functions that call the compiler: we have to pass
requiresas well as the dependencies on of the files of library dependencies. So the last few commits addArg_spec.Hidden_deps, which allows to declare additional dependencies that are not present on the command line. This is used to represent include directory: we can pack together the-Iflags and the file dependencies.