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.
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
Add support for -H compiler flag #10644
Add support for -H compiler flag #10644
Changes from all commits
a4a6bf9
663ec11
42f1798
1bf8bf8
1893536
0c07a77
366ca51
d9df9e4
9fae1f7
3607a91
923a0bc
8e1757f
cdd08f7
c8f4f7f
6ef7f54
51d6e2f
83bb651
2812ddb
80a1c02
9d8c029
c11722d
f1882cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this mode still needs to be guarded behind
implicit_transitive_deps
. Once we experiment with it more, we can enable it by default for newer compilers perhaps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I do not seem to have understood well your point. The new feature is invoked only when
implicit_transitive_deps
is false, according to the written logic, or am I missing something?Thanks for the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that wasn't very clear at all. Let me explain the behavior I would like to achieve:
When we're using
(implicit_transitive_deps false)
and OCaml >= 5.2.0, we should use use your new implementation with the-H
flag.When we're using
(implicit_transitive_deps false)
and OCaml < 5.2.0, we should go back to using the old implementation of hiding-I
flags.When we're using
(implicit_transitive_deps true)
, it should disable using-H
flags completely.The issue with the current behavior is that users do expect upgrading to 3.17 to be relatively easy. With this change, they will now need to look at all their transitive deps. While I agree that it's a good thing, it's too much of a breakage to introduce in a minor version bump.
Therefore, I suggest that we guard this behind a feature flag. We already have a feature flag though
implicit_transitive_deps
, so we can just reuse it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this re-ordering the include flags? e.g. if we have
(libraries x y)
, we'd haveclosure(x) closure(y)
, and now we havex y closure(x) closure(y)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand things correctly, I don't think anything is being re-ordered; instead some extra
-H
flags are being added at the "end" of the command-line whenimplicit_transitive_deps
isfalse
:implicit_transitive_deps
istrue
, thendirect_includes
isrequires_link
(fromCompilation_context
), andhidden_includes
is empty; this is the same as today, and nothing changes.implicit_transitive_deps
isfalse
(and OCaml is recent enough, etc)direct_includes
isrequires_compile
(fromCompilation_context
), andhidden_includes
contains some extra flags relative to today, but the order ofdirect_includes
does not change.Do you agree with this understanding @MA0010?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.