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 BOM / dependency management support #3924
Add BOM / dependency management support #3924
Changes from 27 commits
910e8a4
4f5f070
3e8643d
f796d0c
bc5e1b3
425b4aa
732c659
17259c0
a8b86e3
67ff17a
067d84d
17c3084
c143182
a45ddd6
d601c44
20f59c5
8c00ccb
f53fd8d
c549b21
06b0c5b
1828975
c9054a3
3dbc225
b260f6f
e13a102
9c75f73
97cb113
13fdbca
8e28493
7950d4f
4f78fe8
03857e0
745b500
26b5c54
f286477
8aa498a
61b6061
5bd61aa
73f5c10
e5fe19f
169a747
2a5ca63
26116a9
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.
Can we leave out the
:_
and just useivy"com.google.protobuf:protobuf-java"
? That would make our syntax consistent with Maven and Gradle, both of which just leave out the version segment without any placeholderThere 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.
Trying that
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.
Leaving out the version entirely would also be consistent with how we currently handle missing versions for contrib plugins. We could remove the special handling and instead auto-include a BOM configuring all contrib plugins.
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.
Things like
ivy"com.google.protobuf:protobuf-java"
are now accepted.Note that missing version translates to the version field being
"_"
, which still acts as a placeholder for versions (that makes better error mssages IMO).So we have
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
ivy"com.google.protobuf:protobuf-java:_"
could be arguably read as "any version will do", whereas the missing versions rather means, "version needs to be configured elsewhere and is not provided here".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 we need to make sure the
_
used in coursier does not leak into Mill, which is probably not possible, since we forward coursier output in error messages andivyDepsTree
to name some. Out of curiosity, were there any limitations forcing you to choose_
as a marker? Would it be possible to chnage it in coursier?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 guess it could still be changed in coursier, but for what? I used a marker so that the dependency parsers in coursier don't need much change (compared to accepting things like
org:name
)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.
For the user experience? For the consistency with Maven and Gradle?
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.
And for avoidance of extra work in Mill, since we already use
_
as a ubiquitous wildcard, not as a marker for absence. We only have the choise to_
and off-load the inconsistence to the userThere 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.
So how we want to proceed now? Since
Dep
does not fully abstract over the innerdep: Dependency
, the version is often used as is, and the forwarderDep.verison
wasn't touched in this PR, so we end up with a mix-up of defined or empty or_
versions.