Map packaging to non-optional #2718
Conversation
dwijnand
force-pushed
the
failure-isnt-optional
branch
5 times, most recently
from
55337bd
to
c5bba09
Compare
|
@alexarchambault let me know what you think? I think it's just that typo in MavenRepositoryInternal, and everything else is just the cascade effect of that. I tried to keep the intent of the rest of the tests. |
|
Hey @alexarchambault (or @julienrf) any chance this could be reviewed, merged and released, please? |
|
@dwijnand I'll have a closer look later, but at first sight, this is not a trivial change… Packaging has been optional on purpose. Some dependencies do miss a JAR, and they shouldn't fail. About the repo linked in #2377, I'd need to have a closer look, but at first, I'd assume the repository to be broken. |
|
That said, if I read #2377 (comment) correctly, and if your change is only about the case where the dependency has an explicit |
There was a problem hiding this comment.
IIUC #2377 (comment) there is an inconsistency between the way Coursier reads a dependency from a pom or from the command-line.
From the command-line (e.g. cs fetch javax.media:jai_core:1.1.3,type=jar), when we set the type to jar, Coursier fails to resolve the dependency if the .jar file is absent.
However, when the dependency is read from a pom file with <packaging>jar</packaging>, Coursier marks the .jar file as optional and does not fail the resolution if the file is absent.
This PR fixes this inconsistency, so the approach looks good to me. Please see my other comments below.
AFAIU, this PR may not completely address the linked issue because it will not, by default, expand dependency descriptors to type=jar,optional=false.
| @@ -803,6 +803,7 @@ object InstallTests extends TestSuite { | |||
|
|
|||
| val scala2CompilerJars = | |||
| Seq( | |||
| "https://repo1.maven.org/maven2/org/scala-lang/modules/scala-xml_2.12/1.0.6/scala-xml_2.12-1.0.6.jar", | |||
There was a problem hiding this comment.
Is this change related to the PR?
There was a problem hiding this comment.
Yes, of course. It was previously missing, before this fix.
| @@ -223,7 +223,6 @@ https://repo1.maven.org/maven2/xml-apis/xml-apis/1.3.04/xml-apis-1.3.04.jar | |||
| http://repository.mapr.com/maven/org/apache/hadoop/hadoop-yarn-server-nodemanager/2.7.0-mapr-1808/hadoop-yarn-server-nodemanager-2.7.0-mapr-1808.jar | |||
| http://repository.splicemachine.com/nexus/content/groups/public/com/splicemachine/splice_encoding/2.8.0.1915-SNAPSHOT/splice_encoding-2.8.0.1915-20190427.010754-1.jar | |||
| https://repo1.maven.org/maven2/software/amazon/ion/ion-java/1.0.1/ion-java-1.0.1.jar | |||
| https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.6.7.1/jackson-databind-2.6.7.1.jar | |||
There was a problem hiding this comment.
Why is this change necessary?
There was a problem hiding this comment.
The fix removes the duplicate jackson-databind jar (there's one on line 121).
|
I agree with @julienrf comments. I'd like to take the time to understand why the databind and the scala-xml changes are necessary |
I don't know what that means, outside of not making packaging=jar non-optional.. |
I might be missing something, but what I meant is that with this PR Courser will not set the packaging to jar by default when reading dependencies from a pom. It will still have to be specified explicitly in the pom. Probably, this is fine and should not hold this PR. |
|
I'll try to find time to have a closer look in the coming days (until next Monday say, don't merge until then) |
|
Thank you ❤️ |
alexarchambault
force-pushed
the
failure-isnt-optional
branch
from
c5bba09
to
3ef649f
Compare
|
So I had a closer look at the changes in test fixtures that looked suspicious. #2776 and #2777 deal with those. I just pushed a rebased version of the original code of this PR, and the diff looks much nicer now 🎉 I don't feel like writing a long explanation about this right now, given it's quite late for me 😬 I might tomorrow or later during the week. In any case, thanks for having pushed for this, @dwijnand! I'll merge if the CI gets green (it should), and this'll be part of the next release. |
|
Very nice, thank you. It's green, btw. |
|
@alexarchambault could we merge this, please? |
Fixes #2377. Depends on coursier/test-metadata#8.