[JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem #65

jglick · 2019-04-09T03:52:43Z

Downstream of jenkinsci/structs-plugin#43.

Resolved conflicts with jenkinsci#67.

dwnusbaum · 2019-06-20T19:18:01Z

src/main/java/org/jenkinsci/plugins/workflow/libs/SCMSourceRetriever.java

+            if (scm instanceof UninstantiatedDescribable) {
+                UninstantiatedDescribable scmUd = (UninstantiatedDescribable) scm;
+                Map<String, Object> scmArguments = new HashMap<>(scmUd.getArguments());
+                scmArguments.remove("id");


So IIUC, the problem is that we don't want this ID to be part of the model, since it should be unique to each instance?

Right. SCMSource does a weird trick that does not work like your typical Describable: it generates a random id each time you instantiate it, but also lets the ID be customized.

dwnusbaum · 2019-06-20T19:23:56Z

Rebuilding because the Windows agent had connectivity issues.

dwnusbaum · 2019-06-21T14:28:33Z

pom.xml

        <workflow-cps-plugin.version>2.69</workflow-cps-plugin.version>
-        <git-plugin.version>3.6.4</git-plugin.version>
-        <scm-api-plugin.version>2.2.7</scm-api-plugin.version>
+        <git-plugin.version>4.0.0-rc</git-plugin.version>


PR looks good, but I want to wait for a stable release of git 4.x before merging the change (Commenting so I don't forget about this later). What would happen if we merged this as-is and released to the non-experimental update center? Broken instances for everyone because they can't satisfy the git dependency?

git is only a test-scoped dep, but git-client is a problem because it is in dependencies as compile scope:

Plugin-Dependencies: workflow-cps:2.69,workflow-scm-step:2.4,workflow-step-api:2.19,workflow-support:3.3,cloudbees-folder:6.1.0,credentials:2.1.14,git-client:3.0.0-rc,git-server:1.7,scm-api:2.3.0,script-security:1.58,structs:1.18

I suspect it is only there to satisfy RequireUpperBoundsDeps, in which case it would better be moved to dependencyManagement and then would not appear in the release at all. Would that suffice from your PoV?

Oops, I totally missed git being test-scope. Yeah if moving git-client into dependencyManagement gets it out of compile scope but tests still pass then I'd be fine to merge the change at that point.

OK, will try that.

Ok, merged this a bit prematurely because I did not actually run mvn dependency:tree. This library has a compile-scope dependency on git-server which itself has a compile-scope dependency on git-client, so moving git-client to dependencyManagement does not fix the problem, and so the plugin currently has an experimental release as a compile-scope dependency. I'm not sure to what extent git-server uses git-client or if there is an easy fix for this. Maybe safest to revert the merge for now?

moving git-client to dependencyManagement does not fix the problem

It does because it does not have a direct dependency on git-client, so it does not appear in the manifest.

mvn -DskipTests clean package unzip -p target/workflow-cps-global-lib.hpi META-INF/MANIFEST.MF | perl -p -0777 -e 's/\r?\n //g' | fgrep Plugin-Dependencies

⇒

Plugin-Dependencies: workflow-cps:2.69,workflow-scm-step:2.4,workflow-step-api:2.19,workflow-support:3.3,cloudbees-folder:6.1.0,credentials:2.1.14,git-server:1.7,scm-api:2.3.0,script-security:1.58,structs:1.18

Ok makes sense, in an actual instance the version of git-client required by the plugin will be whatever git-server 1.7 requires because of how Jenkins resolves plugin dependencies.

Well it will be whatever version of git-client the user happens to have installed. git-server only requires 1.11.0, which is quite old. The current version offered on the general UC is 2.8.0.

Lots of other deps could probably be moved to dependencyManagement too; would be useful to have a tool which does this automatically based on source/bytecode scans.

dwnusbaum · 2019-06-21T18:42:01Z

GrapeTest.var failed in the CI build because of a script-security issue, not sure offhand how/if it relates to the changes in this PR:

Scripts not permitted to use method org.apache.commons.math3.FieldElement add java.lang.Object. Administrators can decide whether to approve or reject this signature.

jglick · 2019-06-21T19:01:37Z

Reproducible. Not dealing with it here, but someone is going to need to bisect this stuff to find out why.

dwnusbaum · 2019-06-24T16:52:29Z

someone is going to need to bisect this stuff to find out why.

The problem is the jenkins-test-harness 2.50 update. Using plugin-pom 3.45 with jenkins-test-harness 2.49 the test passes. Looks like the issue is that jenkinsci/jenkins-test-harness#135 added a dependency on jmh-core, which has a transitive dependency on commons-math3, which is the library grabbed in the test, so it breaks things (because it comes from the wrong class loader?). I guess the simplest fix would be to make the test @Grab some other library that is not already on the classpath somewhere to make the test work again.

mvn dependency:tree for jenkins-test-harness master:

[INFO] +- org.openjdk.jmh:jmh-core:jar:1.21:compile
[INFO] |  +- net.sf.jopt-simple:jopt-simple:jar:4.6:compile
[INFO] |  \- org.apache.commons:commons-math3:jar:3.2:compile

basil · 2019-06-25T16:52:19Z

pom.xml

+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>org.jenkins-ci.plugins</groupId>


Lots of other deps could probably be moved to dependencyManagement too; would be useful to have a tool which does this automatically based on source/bytecode scans.

I realize that the Jenkins community is fairly Maven-centric, but FYI Netflix's Gradle Lint Plugin has this feature (c.f. Unused Dependency Rule).

I think it is likely there is a Maven equivalent, I just have not looked for it yet.

I think it is likely there is a Maven equivalent, I just have not looked for it yet.

Looks like the Maven Dependency Plugin can list unused declared dependencies, though (just like the Gradle version) there seem to be too many false positives for it to be usable as part of any kind of automated enforcement.

The problem of false positives is compounded by the heavy use of reflection, Groovy views, etc. etc. in Jenkins sources.

jglick · 2019-06-25T16:57:08Z

Note that as described in JIRA, this will not magically fix things for configuration-as-code, which unfortunately does not use DescribableModel but reimplements a similar reflection system.

CustomDescribableModel can address SCMSourceRetriever.scm.id problem.

f279b0a

jglick added the work-in-progress label Apr 9, 2019

jglick added 2 commits April 9, 2019 09:46

New API.

09c203e

Upper bounds issues.

c098b4f

jglick removed the work-in-progress label Apr 9, 2019

jglick added 2 commits April 24, 2019 10:01

Merge branch 'master' into CustomDescribableModel-JENKINS-44892

cdbcda1

Resolved conflicts with jenkinsci#67.

structs 1.18

a51a7cc

jglick mentioned this pull request Jun 4, 2019

[JENKINS-56682] Reproduce and fix issue using types from shared libraries in Script initializers #71

Merged

Merge branch 'master' into CustomDescribableModel-JENKINS-44892

e137928

jglick requested a review from dwnusbaum June 20, 2019 19:15

dwnusbaum reviewed Jun 20, 2019

View reviewed changes

dwnusbaum approved these changes Jun 20, 2019

View reviewed changes

dwnusbaum closed this Jun 20, 2019

dwnusbaum reopened this Jun 20, 2019

dwnusbaum reviewed Jun 21, 2019

View reviewed changes

jglick added the work-in-progress label Jun 21, 2019

git-client is not a direct dependency.

c456695

Lots of other deps could probably be moved to dependencyManagement too; would be useful to have a tool which does this automatically based on source/bytecode scans.

jglick removed the work-in-progress label Jun 21, 2019

jglick requested a review from dwnusbaum June 21, 2019 17:34

Parent 3.43 → 3.45 breaks GrapeTest.var for reasons TBD.

8cc94b5

dwnusbaum approved these changes Jun 24, 2019

View reviewed changes

dwnusbaum merged commit 95ebc4c into jenkinsci:master Jun 25, 2019

basil reviewed Jun 25, 2019

View reviewed changes

jglick deleted the CustomDescribableModel-JENKINS-44892 branch June 25, 2019 16:55

dwnusbaum changed the title ~~CustomDescribableModel can address SCMSourceRetriever.scm.id problem~~ [JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem Jun 25, 2019

dwnusbaum mentioned this pull request Jun 25, 2019

Make GrapeTest.var work with new versions of jenkins-test-harness #73

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem #65

[JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem #65

jglick commented Apr 9, 2019

dwnusbaum Jun 20, 2019

jglick Jun 20, 2019

dwnusbaum commented Jun 20, 2019

dwnusbaum Jun 21, 2019

jglick Jun 21, 2019

dwnusbaum Jun 21, 2019

jglick Jun 21, 2019

dwnusbaum Jun 26, 2019

jglick Jun 26, 2019

dwnusbaum Jun 27, 2019 •

edited

Loading

jglick Jun 27, 2019 •

edited

Loading

dwnusbaum commented Jun 21, 2019

jglick commented Jun 21, 2019

dwnusbaum commented Jun 24, 2019

basil Jun 25, 2019

jglick Jun 25, 2019

basil Jun 25, 2019

jglick Jun 25, 2019

jglick commented Jun 25, 2019

[JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem #65

[JENKINS-44892] CustomDescribableModel can address SCMSourceRetriever.scm.id problem #65

Conversation

jglick commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

jglick Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

dwnusbaum commented Jun 21, 2019

jglick commented Jun 21, 2019

dwnusbaum commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jun 25, 2019

dwnusbaum Jun 27, 2019 •

edited

Loading

jglick Jun 27, 2019 •

edited

Loading