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

Drop 2.289.x and 2.303.x lines #1289

Merged
merged 8 commits into from Jul 21, 2022
Merged

Conversation

jetersen
Copy link
Member

this is currently blocking due to cycle dependency between plugins

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

this is currently blocking due to cycle dependency between plugins
@jetersen
Copy link
Member Author

I guess this is what we get for not testing every line 😭

@jetersen
Copy link
Member Author

Seems only pct-junit test fails on 2.319.x

@jglick how do you usually go about testing it? So we can find a resolution?

@jetersen
Copy link
Member Author

figured out testing.

Seems to be related to jenkinsci/junit-plugin#362

So either we ignore some tests in junit or backport fix?

@jetersen
Copy link
Member Author

Could also be related to:

Cycle detected: Plugin:jackson2-api -> Plugin:jersey2-api -> Plugin:jackson2-api

Not sure if junit plugin uses jackson2 or if html tests using it.

@jglick jglick changed the title drop 2.289.x and 2.303.x Drop 2.289.x and 2.303.x lines Jul 20, 2022
@jglick
Copy link
Member

jglick commented Jul 20, 2022

FTR https://www.jenkins.io/doc/developer/plugin-development/choosing-jenkins-baseline/#currently-recommended-versions

At the moment, the Jenkins releases 2.319.3 and 2.332.4 make good core dependencies. You could also consider 2.346.1 if there are specific reasons, like new features, to want something newer; 2.289.3 and 2.303.3 would be reasonable conservative choices.

and #1275 (comment)

@jglick
Copy link
Member

jglick commented Jul 20, 2022

Seems to be related to jenkinsci/junit-plugin#362

So either we ignore some tests in junit or backport fix?

I think it is fine to ignore a test in an older line if there is reason to believe it is just a technical failure, not an actual incompatibility.

@jglick
Copy link
Member

jglick commented Jul 20, 2022

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
failFast Outdated Show resolved Hide resolved
@jetersen
Copy link
Member Author

@jglick I have no idea how to read these damn maven logs on jenkins nor locally. 🤯🤯🤯🤯

@jetersen jetersen marked this pull request as draft July 20, 2022 12:54
@jetersen
Copy link
Member Author

okay it is related to the cyclic dependency:

echarts won't load because it depends on jackson2-plugin:
https://github.com/jenkinsci/echarts-api-plugin/blob/b084d4c966427dae373bf8486f3b79dc45ef904f/pom.xml#L116

@jetersen
Copy link
Member Author

@jglick So this goes back to #821 ?

@jglick
Copy link
Member

jglick commented Jul 20, 2022

I do not know. What is the actual plugin loading error?

I think this sort of problem in (non-Real) JenkinsRule tests is sufficient justification for a PCT exclusion: it is clear that the test failure is an artifact of the artificial test environment.

Going forward, I guess we need to be more aggressive about adding to split-plugin-cycles.txt when detaching plugins. Unfortunately it is hard to predict which cycles will be significant and need to be broken. 🤔 Perhaps it would suffice for the plugin manager to avoid adding implied dependencies on detached plugins from other detached plugins?

@jetersen
Copy link
Member Author

jetersen commented Jul 20, 2022

split-plugin-cycles.txt for jersey and jackson had nothing todo with core?

For it to work we need https://github.com/jenkinsci/bom/releases/tag/1280.vd669827e38cd in 1.54

@jetersen
Copy link
Member Author

@jglick is there something sed like I could do in the local-test shell script to verify that bumping bom in junit pom would fix it?

@timja
Copy link
Member

timja commented Jul 20, 2022

@jglick is there something sed like I could do in the local-test shell script to verify that bumping bom in junit pom would fix it?

you should be able to grab the maven test command that is being invoked (it's logged somewhere in the wall of output), let it run and fail once and then edit the local checkout of junit it does in the target folder

@jetersen
Copy link
Member Author

you should be able to grab the maven test command that is being invoked (it's logged somewhere in the wall of output), let it run and fail once and then edit the local checkout of junit it does in the target folder

Once I remove the megawar (which has the wrong plugins due to wrong bom version) it works.

@timja
Copy link
Member

timja commented Jul 20, 2022

you should be able to grab the maven test command that is being invoked (it's logged somewhere in the wall of output), let it run and fail once and then edit the local checkout of junit it does in the target folder

Once I remove the megawar (which has the wrong plugins due to wrong bom version) it works.

you should be able to do what I did above with the megawar as well and test your fix works end 2 end

@jetersen
Copy link
Member Author

jetersen commented Jul 20, 2022

Well I cannot repack the megawar? or I could force? 😓 It seems to do a git checkout during the packing of megawar

@timja
Copy link
Member

timja commented Jul 20, 2022

Well I cannot repack the megawar? or I could force? 😓 It seems to do a git checkout during the packing of megawar

ah erm possibly

@jglick
Copy link
Member

jglick commented Jul 20, 2022

is there something sed like I could do in the local-test shell script to verify that bumping bom in junit pom would fix it?

Sure, you can run any tests you like locally quite easily: https://github.com/jenkinsci/bom/#pct (Note that docs ought to mention you can also override LINE.)

@jetersen
Copy link
Member Author

jetersen commented Jul 20, 2022

@timja could you create a branch of junit plugin from tag junit-1.54

So we could make backport to bump the bom version?

diff --git a/pom.xml b/pom.xml
index 5218cc9..e9603af 100644
--- a/pom.xml
+++ b/pom.xml
@@ -192,7 +192,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.303.x</artifactId>
-                <version>1117.v62a_f6a_01de98</version>
+                <version>1280.vd669827e38cd</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>

You can also pick the latest bom to avoid other detached plugins.

Another option would be to ignore these tests.

Or fix #821

@timja
Copy link
Member

timja commented Jul 20, 2022

Seems to already exist: https://github.com/jenkinsci/junit-plugin/tree/1.54.x

@jetersen
Copy link
Member Author

@timja You created a commit on it that you may remove from history: jenkinsci/junit-plugin@029253b you would want to use MRP to change that right :)

@jglick
Copy link
Member

jglick commented Jul 20, 2022

jenkinsci/junit-plugin@029253b looks wrong @timja: should rather leave version alone and edit revision, as in https://gist.github.com/jglick/86a30894446ed38f918050c1180483e2

@timja
Copy link
Member

timja commented Jul 20, 2022

jenkinsci/junit-plugin@029253b looks wrong @timja: should rather leave version alone and edit revision, as in gist.github.com/jglick/86a30894446ed38f918050c1180483e2

Seems to have worked anyway: jenkinsci/junit-plugin@cb49499

@jglick
Copy link
Member

jglick commented Jul 20, 2022

Seems to have worked anyway

Yeah and jenkinsci/junit-plugin@16fcc7e fixed it up automatically, so net https://github.com/jenkinsci/junit-plugin/compare/1.54.x is reasonable.

@jetersen jetersen requested a review from jglick July 21, 2022 02:06
@jetersen jetersen marked this pull request as ready for review July 21, 2022 02:06
@jetersen
Copy link
Member Author

Would like to land #1275 together with this.

So I would prefer to drop the label until that is merged. WDYT @jglick ? :)

@jetersen jetersen removed the breaking label Jul 21, 2022
@jetersen jetersen merged commit f76b9e8 into jenkinsci:master Jul 21, 2022
@jetersen jetersen deleted the drop/olderLines branch July 21, 2022 09:10
@jglick jglick added the removed label Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants