resolve-test-dependencies fail to update classpath to use some optional transitive dependencies coming from hpi included in overrideWar #391

olamy · 2022-11-07T00:04:55Z

Signed-off-by: Olivier Lamy olamy@apache.org

…al transitive dependencies coming from hpi included in overrideWar Signed-off-by: Olivier Lamy <olamy@apache.org>

…rAdditions Signed-off-by: Olivier Lamy <olamy@apache.org>

Signed-off-by: Olivier Lamy <olamy@apache.org>

src/it/test-dependencies-transitive-optional/pom.xml

src/it/override-test-dependencies-useUpperBounds/pom.xml

olamy · 2022-11-07T09:26:50Z

Unfortunately even if this approach the problem is resolve dependencies too deep and try to get dependencies of bad old poms :( (bad metadata from the past)

src/it/override-test-dependencies-useUpperBounds/pom.xml

src/main/java/org/jenkinsci/maven/plugins/hpi/JenkinsHpiDependencyCollector.java

src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java

Signed-off-by: Olivier Lamy <olamy@apache.org>

jglick

No description?

jglick · 2022-12-21T21:55:40Z

pom.xml

@@ -344,6 +354,9 @@
          <pomIncludes>
            <pomInclude>*/pom.xml</pomInclude>
          </pomIncludes>
+          <setupIncludes>
+            <setupInclude>setup_test-dependencies-transitive-optional-dep/pom.xml</setupInclude>


Does this really need to be done this way, or can you just do everything in one dir using a multimodule reactor?

jglick · 2022-12-21T22:00:04Z

src/it/test-dependencies-transitive-optional/pom.xml

+                        <id>copy-jenkins-war</id>
+                        <phase>process-resources</phase>
+                        <goals>
+                            <goal>copy</goal>


This does not look anything like a real plugin POM so I am struggling to follow what bug this IT purports to reproduce.

jtnord · 2024-03-28T10:53:32Z

as we are here in the case of some optional transitive dependencies.

but optional dependencies are not transitive so should not be present at all (so should not need updating).
Something seems fishy @olamy ? (only direct (non-transitive) optional dependencies should be updated, or I am missing something?)

olamy · 2024-03-29T05:51:45Z

as we are here in the case of some optional transitive dependencies.

but optional dependencies are not transitive so should not be present at all (so should not need updating). Something seems fishy @olamy ? (only direct (non-transitive) optional dependencies should be updated, or I am missing something?)

no of course optional dependencies are not transitive. But it looks there are some discrepancies between Jenkins plugins system (in PCT only?) and Maven dependencies (as you can read them in poms).

What I can see from the current case (issue from jenkinsci/bom#3049)

before

mina-sshd-api-core
+- ssh-credentials
|  \- trilead-api

after

mina-sshd-api-core
+- ssh-credentials
|   @OptionalExtension(requirePlugins = {"trilead-api"})
|  \- trilead-api (optional)

As we can think, optional is not transitive when running mina-sshd-api-core tests in the PCT/bom context, the surefire classpath should not include trilead-api and it doesn't, so the CNFE.
BUT and it looks to be the real problem, while the extension is marked as @OptionalExtension(requirePlugins = {"trilead-api"}) so you can you think as reading the poms this extension shouldn't be loaded as it's transitive optional.
But not Jenkins load because it can see the plugin but on the other side the classpath used by surefire doesn't contain the classes from trilead-api whereas it should.
So either we fix something in how Jenkins loads plugins in this case or we include the trilead-api in the classpath (i.e the transitive optional).
Or as a workaround we keep adding non-needed dependencies in poms just to hide the root cause. (such jenkinsci/mina-sshd-api-plugin#91 and there are probably few more).
But this is a bit of a workaround and doesn't look right while reading the poms. PCT may have some requirements and different way of testing. But loading of plugins/extensions (respecting the annotation @OptionalExtension(requirePlugins = {"xxx"})) should be consistent with the classpath calculation to run the tests.

jglick · 2024-03-29T12:46:34Z

loading of plugins/extensions […] should be consistent with the classpath calculation

In general, if you need to delve into this kind of detail, you should switch to RealJenkinsRule.

jtnord · 2024-05-28T11:48:21Z

root cause of this is jenkinsci/bom#3231
Adding a workaround/fix here would not fully solve the issue and would leave plugins still needing test fixes specific to running in the OSS bom build with a fat war

basil

Given that the essence of all the changes to TestDependencyMojo has been covered in other PRs, is there any reason to keep this PR open?

basil · 2024-10-28T21:34:24Z

src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java

+                        DependencyCollectorRequest dependencyCollectorRequest =
+                                new DependencyCollectorRequest(buildingRequest)
+                                        .dependencySelector(new AndDependencySelector(
+                                                new DirectScopeDependencySelector(JavaScopes.TEST),
+                                                new DirectScopeDependencySelector(JavaScopes.PROVIDED)));
+                        node = dependencyCollectorBuilder.collectDependencyGraph(dependencyCollectorRequest);


Included in #673:

List<org.eclipse.aether.graph.Dependency> dependencies = shadow.getDependencies().stream() .filter(d -> !d.isOptional()) .filter(d -> !List.of(Artifact.SCOPE_TEST, Artifact.SCOPE_PROVIDED) .contains(d.getScope())) .map(d -> RepositoryUtils.toDependency(d, artifactTypeRegistry)) .collect(Collectors.toList());

basil · 2024-10-28T21:35:59Z

src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java

@@ -328,7 +335,7 @@ public void execute() throws MojoExecutionException {
                    continue;
                }

-                getLog().debug("Copying " + artifactId + " as a test dependency");
+                getLog().debug("Copying " + artifactId + ":" + a.getVersion() + " as a test dependency");


Included in #462:

getLog().debug("Copying " + artifact.getGroupId() + ":" + artifact.getArtifactId() + ":" + artifact.getVersion() + " as a test dependency");

basil · 2024-10-28T21:36:51Z

src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java

-                pairs = new ArrayList<>();
-                keyToPairsMap.put(key, pairs);
-            }
+            List<DependencyNodeHopCountPair> pairs = keyToPairsMap.computeIfAbsent(key, s -> new ArrayList<>());


Included in #402:

List<DependencyNodeHopCountPair> pairs = keyToPairsMap.computeIfAbsent(key, unused -> new ArrayList<>());

basil · 2024-10-28T21:37:25Z

src/main/java/org/jenkinsci/maven/plugins/hpi/TestDependencyMojo.java

-                    if (hopPair.getHopCount() < resolvedPair.getHopCount()) {
+                    if (hopPair.getHopCount() < resolvedPair.getHopCount() ||
+                            // we can have dependencies with similar hop count but one is optional and the other not
+                            // Maven will use the non optional even if the version is higher
+                            (hopPair.getHopCount() == resolvedPair.getHopCount() &&
+                            resolvedPair.getNode().getArtifact().isOptional()) &&
+                            !hopPair.getNode().getArtifact().isOptional()) {


Deleted upstream and so also deleted in #673.

olamy · 2024-10-30T08:37:31Z

good to see the changes have been integrated smoothly via other PRs

jenkinsci#3158 provides the fundamental information for the pull request. jenkinsci/maven-hpi-plugin#391 provides additional commentary. Also removes trailing spaces from lines in the file.

#3158 provides the fundamental information for the pull request. jenkinsci/maven-hpi-plugin#391 provides additional commentary. Also removes trailing spaces from lines in the file.

resolve-test-dependencies fail to update classpath to use some option…

ea01d86

…al transitive dependencies coming from hpi included in overrideWar Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy added the bug label Nov 7, 2022

olamy marked this pull request as draft November 7, 2022 00:26

olamy added the work-in-progress label Nov 7, 2022

olamy added 3 commits November 7, 2022 10:50

force inclusion of support-core jar to reproduce not using overrideWa…

259bf57

…rAdditions Signed-off-by: Olivier Lamy <olamy@apache.org>

not needed

df62a8d

some exclusions of bad poms...

0a2bccf

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy closed this Nov 7, 2022

olamy reopened this Nov 7, 2022

olamy removed the work-in-progress label Nov 7, 2022

olamy commented Nov 7, 2022

View reviewed changes

src/it/test-dependencies-transitive-optional/pom.xml Show resolved Hide resolved

olamy commented Nov 7, 2022

View reviewed changes

src/it/override-test-dependencies-useUpperBounds/pom.xml Show resolved Hide resolved

jtnord reviewed Nov 7, 2022

View reviewed changes

upgrade maven-dependency-tree

7c5299e

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy marked this pull request as ready for review November 19, 2022 11:07

jglick requested a review from jtnord December 21, 2022 21:52

jglick reviewed Dec 21, 2022

View reviewed changes

basil mentioned this pull request Mar 28, 2023

Log more artifact metadata in TestDependencyMojo #462

Merged

basil mentioned this pull request Nov 23, 2023

Use parent pom requireUpperBoundDeps configuration jenkinsci/analysis-pom-plugin#826

Closed

6 tasks

uhafner mentioned this pull request Dec 4, 2023

Set the version of org.junit.platform to 1.10.1 jenkinsci/analysis-pom-plugin#834

Merged

strangelookingnerd mentioned this pull request Jan 8, 2024

custom-folder-icon 2.11 fails tests in plugin bill of materials jenkinsci/custom-folder-icon-plugin#280

Closed

This was referenced Mar 28, 2024

bump minimum baseline to 2.387.3 and use ssh-credentials which mark trilead-api as optional and add trilead-api as optional jenkinsci/mina-sshd-api-plugin#89

Merged

Revert ssh-credentials that made trilead-api optional jenkinsci/bom#3049

Merged

jtnord mentioned this pull request Mar 28, 2024

mark trilead api as test (even if we should not need this) jenkinsci/mina-sshd-api-plugin#91

Closed

basil mentioned this pull request May 1, 2024

Improve maintainer documentation jenkinsci/bom#3158

Open

olamy mentioned this pull request May 28, 2024

add bouncycastle / eddsa plugins as optional and remove trilead jenkinsci/mina-sshd-api-plugin#98

Merged

MarkEWaite mentioned this pull request Jul 25, 2024

Use plugin bom version prior to trilead API upgrade jenkinsci/ssh-credentials-plugin#212

Merged

6 tasks

strangelookingnerd mentioned this pull request Aug 28, 2024

Remove <optional> from config-file-provider dependency jenkinsci/job-dsl-plugin#1414

Closed

basil reviewed Oct 28, 2024

View reviewed changes

olamy closed this Oct 30, 2024

MarkEWaite mentioned this pull request Nov 16, 2024

Describe 2 known issues and link to example pull requests jenkinsci/bom#3980

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve-test-dependencies fail to update classpath to use some optional transitive dependencies coming from hpi included in overrideWar #391

resolve-test-dependencies fail to update classpath to use some optional transitive dependencies coming from hpi included in overrideWar #391

olamy commented Nov 7, 2022

olamy commented Nov 7, 2022

jglick left a comment

jglick Dec 21, 2022

jglick Dec 21, 2022

jtnord commented Mar 28, 2024 •

edited

Loading

olamy commented Mar 29, 2024 •

edited

Loading

jglick commented Mar 29, 2024

jtnord commented May 28, 2024

basil left a comment

basil Oct 28, 2024

basil Oct 28, 2024

basil Oct 28, 2024

basil Oct 28, 2024

olamy commented Oct 30, 2024

resolve-test-dependencies fail to update classpath to use some optional transitive dependencies coming from hpi included in overrideWar #391

resolve-test-dependencies fail to update classpath to use some optional transitive dependencies coming from hpi included in overrideWar #391

Conversation

olamy commented Nov 7, 2022

olamy commented Nov 7, 2022

jglick left a comment

Choose a reason for hiding this comment

jglick Dec 21, 2022

Choose a reason for hiding this comment

jglick Dec 21, 2022

Choose a reason for hiding this comment

jtnord commented Mar 28, 2024 • edited Loading

olamy commented Mar 29, 2024 • edited Loading

jglick commented Mar 29, 2024

jtnord commented May 28, 2024

basil left a comment

Choose a reason for hiding this comment

basil Oct 28, 2024

Choose a reason for hiding this comment

basil Oct 28, 2024

Choose a reason for hiding this comment

basil Oct 28, 2024

Choose a reason for hiding this comment

basil Oct 28, 2024

Choose a reason for hiding this comment

olamy commented Oct 30, 2024

jtnord commented Mar 28, 2024 •

edited

Loading

olamy commented Mar 29, 2024 •

edited

Loading