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

Update provisio-maven-plugin to 1.0.20 #16191

Merged
merged 1 commit into from
Jul 2, 2023

Conversation

kokosing
Copy link
Member

Update provisio-maven-plugin to 1.0.20

@cla-bot cla-bot bot added the cla-signed label Feb 20, 2023
@kokosing kokosing requested review from findepi, ebyhr, hashhar and electrum and removed request for findepi February 20, 2023 20:38
@hashhar
Copy link
Member

hashhar commented Feb 21, 2023

Seems to have hit a case where we don't have all dependencies downloaded but we're running maven with --offline. cc: @nineinchnick

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % confirming CI indeed works

@nineinchnick
Copy link
Member

I think we should remove the --offline flag and allow Maven to download missing dependencies when necessary.

I see there are multiple versions of plexus-java being downloaded, and the tools we use to download dependencies might not properly handle that. Before we'll be able to track down the root cause and fix it, we should not be blocked. cc @findepi because you insisted on adding it.

@kokosing
Copy link
Member Author

Retry does not help.

@ebyhr ebyhr removed their request for review February 22, 2023 07:23
@kokosing kokosing force-pushed the origin/master/175_- branch from b0d7764 to f1de430 Compare February 22, 2023 12:16
@kokosing
Copy link
Member Author

@nineinchnick I have rebased and the issue is still there. Is this related to this PR or is it that highly flaky?

@nineinchnick
Copy link
Member

This PR introduces a new version of a dependency that doesn't get downloaded by the commands we execute in download-maven-dependencies.sh. You either have to add it as an exception there (download it explicitly) or remove --offline in these places:

@kokosing
Copy link
Member Author

You either have to add it as an exception there (download it explicitly)

How?

@nineinchnick
Copy link
Member

You either have to add it as an exception there (download it explicitly)

How?

./mvnw dependency:get -Dartifact=org.codehaus.plexus:plexus-java:1.1.1:jar -Dtransitive=false

@kokosing kokosing force-pushed the origin/master/175_- branch 3 times, most recently from 92756c9 to 6bb56e2 Compare February 26, 2023 21:58
@kokosing
Copy link
Member Author

remove --offline in these places:

I have tried with the other option and so far no luck. I am now more inclined to remove the --offline switch. Possibly I need to create a separate PR. First I wanted to get your opinion about it: @nineinchnick and @findepi ?

@nineinchnick
Copy link
Member

I saw you tried explicitly downloading provisio, not that transitive dependency the ci is complaining about, as I was suggesting. I'm ok with removing --offline but let's wait for @findepi.

@findepi
Copy link
Member

findepi commented Mar 10, 2023

@nineinchnick i don't understand yet what we're blocked on. If I understand how --offline works, it provides us with determinism. Before we go offline, we use retries. Once we go offline, we don't need retries. Removing --offline brings us back to less predictibility.

What are we gaining? Why is this new provisio version special?

@nineinchnick
Copy link
Member

@nineinchnick i don't understand yet what we're blocked on. If I understand how --offline works, it provides us with determinism. Before we go offline, we use retries. Once we go offline, we don't need retries. Removing --offline brings us back to less predictibility.

What are we gaining? Why is this new provisio version special?

We can't figure out how to download all required dependencies. I have the same problem in #12929 now.

@nineinchnick
Copy link
Member

@kokosing can you try adding it explicitly in the configuration for the go-offline-maven-plugin?

diff --git a/pom.xml b/pom.xml
index 5887dfdc09..922d798cf8 100644
--- a/pom.xml
+++ b/pom.xml
@@ -2148,6 +2148,12 @@
                                 <version>${dep.drift.version}</version>
                                 <repositoryType>PLUGIN</repositoryType>
                             </DynamicDependency>
+                            <DynamicDependency>
+                                <groupId>ca.vanzyl.provisio.maven.plugins</groupId>
+                                <artifactId>provisio-maven-plugin</artifactId>
+                                <version>1.0.20</version>
+                                <repositoryType>PLUGIN</repositoryType>
+                            </DynamicDependency>
                             <DynamicDependency>
                                 <groupId>com.google.errorprone</groupId>
                                 <artifactId>error_prone_core</artifactId>

@findepi
Copy link
Member

findepi commented Mar 10, 2023

@nineinchnick how did we come up with this list

trino/pom.xml

Lines 2132 to 2226 in a400eaf

<dynamicDependencies>
<DynamicDependency>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.2.0</version>
<repositoryType>PLUGIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.apache.yetus</groupId>
<artifactId>audience-annotations</artifactId>
<version>0.8.0</version>
<repositoryType>PLUGIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>io.airlift.drift</groupId>
<artifactId>drift-javadoc</artifactId>
<version>${dep.drift.version}</version>
<repositoryType>PLUGIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_core</artifactId>
<version>${dep.errorprone.version}</version>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<version>5.3.2</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-params</artifactId>
<version>5.3.2</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.28.2</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-reflect</artifactId>
<version>2.0.5</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.testng</groupId>
<artifactId>testng</artifactId>
<version>5.10</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<version>3.9.1</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-library</artifactId>
<version>1.3</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.easytesting</groupId>
<artifactId>fest-assert</artifactId>
<version>1.4</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>pom</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
<DynamicDependency>
<groupId>org.apache.maven.surefire</groupId>
<artifactId>surefire-testng</artifactId>
<version>2.22.2</version> <!-- legacy version which Surefire seems to need for some reason -->
<type>jar</type>
<repositoryType>MAIN</repositoryType>
</DynamicDependency>
?
why can't we refresh it now?

@kokosing kokosing force-pushed the origin/master/175_- branch from 6bb56e2 to aba9414 Compare March 11, 2023 07:17
@nineinchnick
Copy link
Member

This caused a new error when downloading dependencies:

2023-03-11T07:19:27.4286197Z [INFO] --- go-offline-maven-plugin:1.2.8:resolve-dependencies (default-cli) @ trino-root ---
2023-03-11T07:19:33.4363449Z [ERROR] Error resolving plugin io.takari.maven.plugins:takari-lifecycle-plugin
2023-03-11T07:19:33.4364197Z [ERROR] version can neither be null, empty nor blank

I wonder if this is caused by how provisio calls takari-lifecycle-plugin and if this can be fixed by again adding it explicitly to dynamicDependencies.

@kokosing kokosing force-pushed the origin/master/175_- branch from 03031aa to 2248c39 Compare May 23, 2023 13:25
@kokosing kokosing force-pushed the origin/master/175_- branch from 2248c39 to ffdd4d3 Compare July 2, 2023 06:15
@kokosing kokosing merged commit 56a3776 into trinodb:master Jul 2, 2023
@github-actions github-actions bot added this to the 421 milestone Jul 2, 2023
@kokosing kokosing deleted the origin/master/175_- branch July 10, 2023 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants