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

upgrade maven-shade-plugin version to 3.5.2 #12712

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Mar 23, 2024

Upgrade maven-shade-plugin version from 3.2.x to 3.5.2.
By default, modules are not building the shaded jar.
For the modules require shaded jars, users can specify <shade.phase.prop>package</shade.phase.prop> in the pom.xml file properties section.

@xiangfu0 xiangfu0 added dependencies Pull requests that update a dependency file build labels Mar 23, 2024
@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch 4 times, most recently from 34c0fbc to 07f55c5 Compare March 25, 2024 10:30
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.17%. Comparing base (59551e4) to head (2e74761).
Report is 330 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12712      +/-   ##
============================================
+ Coverage     61.75%   62.17%   +0.42%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2502      +66     
  Lines        133233   136486    +3253     
  Branches      20636    21122     +486     
============================================
+ Hits          82274    84866    +2592     
- Misses        44911    45357     +446     
- Partials       6048     6263     +215     
Flag Coverage Δ
custom-integration1 <0.01% <ø> (-0.01%) ⬇️
integration <0.01% <ø> (-0.01%) ⬇️
integration1 <0.01% <ø> (-0.01%) ⬇️
integration2 0.00% <ø> (ø)
java-11 62.12% <ø> (+0.41%) ⬆️
java-21 62.05% <ø> (+0.43%) ⬆️
skip-bytebuffers-false 62.15% <ø> (+0.40%) ⬆️
skip-bytebuffers-true 62.01% <ø> (+34.28%) ⬆️
temurin 62.17% <ø> (+0.42%) ⬆️
unittests 62.17% <ø> (+0.42%) ⬆️
unittests1 46.74% <ø> (-0.15%) ⬇️
unittests2 27.93% <ø> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch from 07f55c5 to 4eddca6 Compare March 25, 2024 17:33
@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch 9 times, most recently from bba0e19 to 1e727fc Compare March 29, 2024 21:37
@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch 3 times, most recently from 79a56b2 to 15b556f Compare April 18, 2024 13:20
@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch 2 times, most recently from d4a892a to 736360f Compare April 18, 2024 15:27
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -214,7 +214,6 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
<version>3.1.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to keep this plugin? I guess we should never package pinot-perf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not packaged to pinot-distribution, it's trying to generate another main class for perf testing.

@@ -35,6 +35,6 @@
<url>https://pinot.apache.org/</url>
<properties>
<pinot.root>${basedir}/../../..</pinot.root>
<phase.prop>none</phase.prop>
<shade.phase.prop>none</shade.phase.prop>
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) Not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true

<id>build-shaded-jar</id>
<activation>
<property>
<name>skipShade</name>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to keep this option in the main pom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the usage for this for a while.

@@ -2209,6 +2211,9 @@
<!-- Exclude build targets -->
<exclude>**/target/**</exclude>

<!-- Exclude Maven plugin generated files -->
<exclude>**/dependency-reduced-pom.xml</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

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

I never saw this file generated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new version plugin generates this file

@xiangfu0 xiangfu0 force-pushed the upgrade-maven-shade-plugin-version branch from 736360f to 2e74761 Compare April 18, 2024 17:13
@xiangfu0 xiangfu0 merged commit 7a4c0b8 into apache:master Apr 18, 2024
22 checks passed
@xiangfu0 xiangfu0 deleted the upgrade-maven-shade-plugin-version branch April 18, 2024 22:28
Comment on lines -182 to -187
<profiles>
<profile>
<id>build-shaded-jar</id>
<activation>
<activeByDefault>false</activeByDefault>
</activation>
Copy link
Contributor

Choose a reason for hiding this comment

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

After removing this these module level profile the mvn build stopped to publish shaded jars for spi which is required by our build pipeline. We should probably add it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants