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

Flatten quarkus-bom #15899

Merged
merged 1 commit into from Mar 22, 2021
Merged

Conversation

aloubyansky
Copy link
Member

Flatten quarkus-bom with io.quarkus:quarkus-platform-bom-maven-plugin's flatten-platform-bom goal. This goal generates a POM file containing the effective set of the managed dependencies of the original BOM ordered alphabetically (although there is a parameter to preserve the original ordering) with the exception of the Quarkus platform artifacts (such as the json descriptor and properties) that are moved to the top of the BOM.

@aloubyansky aloubyansky requested a review from gsmet March 19, 2021 22:16
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Mar 19, 2021
@maxandersen
Copy link
Contributor

why would we want it sorted alphabeticvally and not by ordering as that is/can be significant ?

will this not affect origins in new platform metadata if we start flattening the boms or is that what you refer to about platform being an exception ?

@gastaldi
Copy link
Contributor

I'm curious: can you elaborate on how ordering can be significant on a flattened BOM?

@maxandersen
Copy link
Contributor

was about to say it defines the order on the classpath - so one can shadow the other...but I then realize if its used as bom pom then that doesn't happen.

@aloubyansky
Copy link
Member Author

why would we want it sorted alphabeticvally and not by ordering as that is/can be significant ?

Simply to make it more human friendly.

will this not affect origins in new platform metadata if we start flattening the boms or is that what you refer to about platform being an exception ?

Correct. This is platform friendly as well.

@famod
Copy link
Member

famod commented Mar 20, 2021

Are you guys sure that ordering is not important here? I think it is, especially for conflicting transitive versions.

Btw, CI failed big time.

@famod
Copy link
Member

famod commented Mar 20, 2021

PS: I guess IDEs like Eclipse won't pick up the flattened BOM, right? (when the BOM module is imported as a project)

@maxandersen
Copy link
Contributor

Wdym picked up?

@famod
Copy link
Member

famod commented Mar 20, 2021

AFAIK, workspace resolution in Eclipse won't be using the flattened BOM from target in modules that depend on it (/import it). It'll probably use the regular unflattened one.

@aloubyansky
Copy link
Member Author

AFAIK, workspace resolution in Eclipse won't be using the flattened BOM from target in modules that depend on it (/import it). It'll probably use the regular unflattened one.

Let's clarify, the flattened bom in the Quarkus repo specifically. Content-wise, It has no changes compared to its original version.

@aloubyansky
Copy link
Member Author

Are you guys sure that ordering is not important here? I think it is, especially for conflicting transitive versions.

The ordering in the BOM or dependencyManagement in general?? I'd be very much surprised. It's ordering of the actual dependencies in the pom that is significant, afaik.

Btw, CI failed big time.

I'll investigate that.

@famod
Copy link
Member

famod commented Mar 20, 2021

The ordering in the BOM or dependencyManagement in general??

dependencyManagement in general. But since a BOM is also just a collection of dependencyManagement entries, I do see an implication there as well.

Sorry, not much time ATM. I'll have to take a look at the resulting BOM before making any further assumptions...

@famod
Copy link
Member

famod commented Mar 20, 2021

I think the main problem in CI is this:

Warning:  The POM for io.quarkus:quarkus-core-deployment:jar:999-SNAPSHOT is invalid, transitive dependencies (if any) will not be available, enable debug logging for more details

@aloubyansky
Copy link
Member Author

The ordering in the BOM or dependencyManagement in general??

dependencyManagement in general. But since a BOM is also just a collection of dependencyManagement entries, I do see an implication there as well.

A BOM is nothing more than a dependencyManagement which is nothing more than a set of version constraints.

@aloubyansky
Copy link
Member Author

[INFO] --- quarkus-maven-plugin:999-SNAPSHOT:generate-code (default) @ quarkus-cli ---
[WARNING] The POM for io.quarkus:quarkus-core-deployment:jar:999-SNAPSHOT is invalid, transitive dependencies (if any) will not be available: 1 problem was encountered while building the effective model for io.quarkus:quarkus-core-deployment:999-SNAPSHOT
[ERROR] 'dependencies.dependency.version' for io.quarkus.gizmo:gizmo:test-jar is missing. @ 

@aloubyansky
Copy link
Member Author

Apparently,

            <dependency>
                <groupId>io.quarkus.gizmo</groupId>
                <artifactId>gizmo</artifactId>
                <type>test-jar</type>
                <version>${gizmo.version}</version>
            </dependency>

is flattened to

      <dependency>
        <groupId>io.quarkus.gizmo</groupId>
        <artifactId>gizmo</artifactId>
        <version>1.0.7.Final</version>
        <classifier>tests</classifier>
      </dependency>

And I am not sure how to fix that properly, atm.

@aloubyansky
Copy link
Member Author

Ok, I figured out where to get the proper type from but it reports the classifier as tests.

@aloubyansky
Copy link
Member Author

According to https://maven.apache.org/guides/mini/guide-attached-tests.html the test-jar dependency is supposed to be accompanied with the classifier tests.

@aloubyansky
Copy link
Member Author

I'm tempted to simply duplicate test-jar constraints: one with the tests classifier and one w/o.

@aloubyansky
Copy link
Member Author

That appears to work. I am going with that.

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked a few scenarios I had in mind (with a sample project) and my concerns have now vanished!
I'm not sure whether I had some old Maven (pre-3.6 or even pre-3.5) behavior in mind or it was simply a bug in my matrix. 😉

TL;DR: LGTM

pom.xml Outdated
<id>quarkus</id>
<name>Quarkus Community</name>
<organization>Quarkus</organization>
<organizationUrl>http://quarkus.io</organizationUrl>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the nitpicking but this should be https.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@aloubyansky aloubyansky merged commit 04db442 into quarkusio:main Mar 22, 2021
@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants