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

JENKINS-38736 Update guice to final 4.0 release. #2568

Merged
merged 1 commit into from Oct 28, 2016

Conversation

5 participants
@KostyaSha
Member

KostyaSha commented Sep 26, 2016

Checking PR build
Diff: google/guice@4.0-beta5...4.0
Previous update attempt #1453


This change is Reviewable

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Sep 26, 2016

Member

@jglick was your bug finally fixed?

Member

KostyaSha commented Sep 26, 2016

@jglick was your bug finally fixed?

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Sep 26, 2016

Member

If it pass fine, will open JIRA issue.

Member

KostyaSha commented Sep 26, 2016

If it pass fine, will open JIRA issue.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Sep 27, 2016

Member

Why are you linking to the diff from 4.0-beta5 rather than 4.0-beta, which is the current release?

Unfortunately the 4.0-beta...4.0 diff doesn't load for me, so probably considerably larger.

Member

daniel-beck commented Sep 27, 2016

Why are you linking to the diff from 4.0-beta5 rather than 4.0-beta, which is the current release?

Unfortunately the 4.0-beta...4.0 diff doesn't load for me, so probably considerably larger.

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Sep 27, 2016

Member

Because beta5 had bug and the question is whether it was fixed.

Member

KostyaSha commented Sep 27, 2016

Because beta5 had bug and the question is whether it was fixed.

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Oct 5, 2016

Member

That affects java8 support.

Member

KostyaSha commented Oct 5, 2016

That affects java8 support.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 5, 2016

Member

IMHO it's definitely reasonable to merge it into the Weekly since tests do not fail.
If something blows up in the corner case, we can consider reverting in the worst case

Member

oleg-nenashev commented Oct 5, 2016

IMHO it's definitely reasonable to merge it into the Weekly since tests do not fail.
If something blows up in the corner case, we can consider reverting in the worst case

@KostyaSha KostyaSha changed the title from [WIP] Update guice to final 4.0 release. to JENKINS-38736 Update guice to final 4.0 release. Oct 5, 2016

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Oct 5, 2016

Member

Created issue in case of regressions https://issues.jenkins-ci.org/browse/JENKINS-38736

Member

KostyaSha commented Oct 5, 2016

Created issue in case of regressions https://issues.jenkins-ci.org/browse/JENKINS-38736

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Oct 5, 2016

Member

was your bug finally fixed?

Which one? At this point I have little recollection of the earlier update.

Member

jglick commented Oct 5, 2016

was your bug finally fixed?

Which one? At this point I have little recollection of the earlier update.

@jglick

jglick approved these changes Oct 5, 2016

As @oleg-nenashev said, I think it is reasonable to merge it, and if we find any problems, investigate.

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Oct 5, 2016

Member

@jglick you reverted beta5 to beta in 00c858d

Member

KostyaSha commented Oct 5, 2016

@jglick you reverted beta5 to beta in 00c858d

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Oct 5, 2016

Member

And then you created google/guice#899

Member

KostyaSha commented Oct 5, 2016

And then you created google/guice#899

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Oct 5, 2016

Member

I have no idea if that bug was fixed or not. Can you please try the test case I mentioned there using the final release?

Member

jglick commented Oct 5, 2016

I have no idea if that bug was fixed or not. Can you please try the test case I mentioned there using the final release?

@jglick

Nope, test case still fails I am afraid.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Oct 5, 2016

Member

Request that jenkinsci/maven-plugin#80 (or its equivalent) be merged and released before this is merged.

Member

jglick commented Oct 5, 2016

Request that jenkinsci/maven-plugin#80 (or its equivalent) be merged and released before this is merged.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 9, 2016

Member

jenkinsci/maven-plugin#80 has been merged, but there is no release by now (CC @aheritier). Without it we cannot integrate this change

Member

oleg-nenashev commented Oct 9, 2016

jenkinsci/maven-plugin#80 has been merged, but there is no release by now (CC @aheritier). Without it we cannot integrate this change

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Oct 9, 2016

Member

@KostyaSha @oleg-nenashev @jglick how is it urgent ?
If I don't have too much things this week I can try to release it. There is just one or two PR that I would like to process before.

Member

aheritier commented Oct 9, 2016

@KostyaSha @oleg-nenashev @jglick how is it urgent ?
If I don't have too much things this week I can try to release it. There is just one or two PR that I would like to process before.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 9, 2016

Member

@aheritier I suppose it's too late for the today's (?) weekly release, but it would be really great if we get it done by the next weekly. Then it will be better soaked before the next LTS baseline, which will likely include the version with updated Guice.

Member

oleg-nenashev commented Oct 9, 2016

@aheritier I suppose it's too late for the today's (?) weekly release, but it would be really great if we get it done by the next weekly. Then it will be better soaked before the next LTS baseline, which will likely include the version with updated Guice.

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Oct 9, 2016

Member

Yes too risky to start a release at midnight :-) Also I have to release maven-interceptors
The breakage of maven jobs is bad enough to hold this merge? Even if I release it in few days ?

Member

aheritier commented Oct 9, 2016

Yes too risky to start a release at midnight :-) Also I have to release maven-interceptors
The breakage of maven jobs is bad enough to hold this merge? Even if I release it in few days ?

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 9, 2016

Member

The breakage of maven jobs is bad enough to hold this merge? Even if I release it in few days ?

Even if it is a Weekly release, I would not like to intentionally break the stuff. Especially since there is no need to get it released ASAP.

Member

oleg-nenashev commented Oct 9, 2016

The breakage of maven jobs is bad enough to hold this merge? Even if I release it in few days ?

Even if it is a Weekly release, I would not like to intentionally break the stuff. Especially since there is no need to get it released ASAP.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Oct 17, 2016

Member

@oleg-nenashev @aheritier We need to point out the potential breakage with old Maven Plugin versions here in the changelog. AFAIU new Jenkins + old Maven plugin = broken. Please correct me if I'm wrong.

Also, we need to think about what to do with the detached Maven Plugin version. We currently include 2.7.1, and it looks like we need to update that, otherwise it'll be broken in case of an upgrade. AFAIR we now enforce upgrades to bundled versions if outdated, so I'd be more comfortable with a 2.7.2.

Member

daniel-beck commented Oct 17, 2016

@oleg-nenashev @aheritier We need to point out the potential breakage with old Maven Plugin versions here in the changelog. AFAIU new Jenkins + old Maven plugin = broken. Please correct me if I'm wrong.

Also, we need to think about what to do with the detached Maven Plugin version. We currently include 2.7.1, and it looks like we need to update that, otherwise it'll be broken in case of an upgrade. AFAIR we now enforce upgrades to bundled versions if outdated, so I'd be more comfortable with a 2.7.2.

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Oct 17, 2016

Member

@daniel-beck yes this is my understanding. In core changelog we should have

JENKINS-38736 Update guice to final 4.0 release => Requires upgrade of maven-plugin to version >= 2.14

detached Maven Plugin version

Are you talking about these PRs ?

#2352
jenkinsci/maven-plugin#72

if ready for these one we'll need to have a maven-plugin 3.0 released with a dependency to maven-builder 1.0 and jenkins 2.x (where the merge is done). This will be another case where the core upgrade will require an upgrade of the maven-plugin ( >= 3.0 in this case). The maven-builder plugin will have to be added in the recommended list of plugins (like gradle, ..) but should be proposed only for this jenkins >= 2.x ( I don't know if it is possible). If installed in oldest versions I think it may not play well together.

Member

aheritier commented Oct 17, 2016

@daniel-beck yes this is my understanding. In core changelog we should have

JENKINS-38736 Update guice to final 4.0 release => Requires upgrade of maven-plugin to version >= 2.14

detached Maven Plugin version

Are you talking about these PRs ?

#2352
jenkinsci/maven-plugin#72

if ready for these one we'll need to have a maven-plugin 3.0 released with a dependency to maven-builder 1.0 and jenkins 2.x (where the merge is done). This will be another case where the core upgrade will require an upgrade of the maven-plugin ( >= 3.0 in this case). The maven-builder plugin will have to be added in the recommended list of plugins (like gradle, ..) but should be proposed only for this jenkins >= 2.x ( I don't know if it is possible). If installed in oldest versions I think it may not play well together.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Oct 18, 2016

Member

otherwise it'll be broken in case of an upgrade

Only in case of someone updating directly to our new core weekly from Hudson 1.295 or earlier. So I am not terribly worried about that.

Member

jglick commented Oct 18, 2016

otherwise it'll be broken in case of an upgrade

Only in case of someone updating directly to our new core weekly from Hudson 1.295 or earlier. So I am not terribly worried about that.

@jglick

jglick approved these changes Oct 18, 2016

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Oct 18, 2016

Member

detached Maven Plugin version

Are you talking about these PRs ?

No, I think @daniel-beck was referring to this. Whose main purpose at this point is to ensure that plugins with a very old core dependency can still link against hudson.maven.* types.

It also controls behavior when Jenkins is upgraded from a truly ancient version, but as I said that is a very minor issue compared to the quite probable issue of something upgrading from one weekly to the next and not upgrading the Maven plugin—and there is nothing we can do about that, since the Jenkins plugin manager does not allow us to express the fact that a particular plugin version will not work with a newer version of some dependency than what it originally requested: we have no mechanism for marking incompatible changes.

Member

jglick commented Oct 18, 2016

detached Maven Plugin version

Are you talking about these PRs ?

No, I think @daniel-beck was referring to this. Whose main purpose at this point is to ensure that plugins with a very old core dependency can still link against hudson.maven.* types.

It also controls behavior when Jenkins is upgraded from a truly ancient version, but as I said that is a very minor issue compared to the quite probable issue of something upgrading from one weekly to the next and not upgrading the Maven plugin—and there is nothing we can do about that, since the Jenkins plugin manager does not allow us to express the fact that a particular plugin version will not work with a newer version of some dependency than what it originally requested: we have no mechanism for marking incompatible changes.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Oct 18, 2016

Member

@jglick

Mostly I am referring to

<maven-plugin.version>2.7.1</maven-plugin.version>

Only in case of someone updating directly to our new core weekly from Hudson 1.295 or earlier. So I am not terribly worried about that.

IIRC since 2.0 we're upgrading all plugins unconditionally to the bundled version. So anyone who installed 1.x and kept upgrading will have a broken version installed. At the same time we could use this to take care of this by referencing a version of the maven-plugin that includes the fix.

@aheritier How difficult would it be to create a 2.7.2 with the fix, and bundle that with Jenkins?

Member

daniel-beck commented Oct 18, 2016

@jglick

Mostly I am referring to

<maven-plugin.version>2.7.1</maven-plugin.version>

Only in case of someone updating directly to our new core weekly from Hudson 1.295 or earlier. So I am not terribly worried about that.

IIRC since 2.0 we're upgrading all plugins unconditionally to the bundled version. So anyone who installed 1.x and kept upgrading will have a broken version installed. At the same time we could use this to take care of this by referencing a version of the maven-plugin that includes the fix.

@aheritier How difficult would it be to create a 2.7.2 with the fix, and bundle that with Jenkins?

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Oct 18, 2016

Member

@daniel-beck How to discover that there was a 2.7.1 not documented ( https://wiki.jenkins-ci.org/display/JENKINS/Maven+Project+Plugin ) :(

Backporting jenkinsci/maven-plugin#80 in a 2.7.2 seems easy (sisu 0.3.3 is Java 6 compatible) the problem is more with the quality of tests/build that may make it fun.

But I don't get the point, why don't we just bundle in jenkins 2.x a recent version of the maven-plugin ?

Member

aheritier commented Oct 18, 2016

@daniel-beck How to discover that there was a 2.7.1 not documented ( https://wiki.jenkins-ci.org/display/JENKINS/Maven+Project+Plugin ) :(

Backporting jenkinsci/maven-plugin#80 in a 2.7.2 seems easy (sisu 0.3.3 is Java 6 compatible) the problem is more with the quality of tests/build that may make it fun.

But I don't get the point, why don't we just bundle in jenkins 2.x a recent version of the maven-plugin ?

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 23, 2016

Member

But I don't get the point, why don't we just bundle in jenkins 2.x a recent version of the maven-plugin ?

I do not see a reason to keep 2.7.x as well. Upgrade to 2.7.1 has been done from 2.5 (2d46bd5), hence there is no stabilization branch approach. 2.7.1 has been likely released from the security fix branch instead of the master. We just need to ensure that the Security fix is in the master.

@aheritier @daniel-beck @KostyaSha Would be great to accelerate this PR, because there is a new LTS line on the horizon. If the change wants to get there, it needs to be soaked as much as possible. Or we can put the PR on hold and merge it when the new LTS gets selected

Member

oleg-nenashev commented Oct 23, 2016

But I don't get the point, why don't we just bundle in jenkins 2.x a recent version of the maven-plugin ?

I do not see a reason to keep 2.7.x as well. Upgrade to 2.7.1 has been done from 2.5 (2d46bd5), hence there is no stabilization branch approach. 2.7.1 has been likely released from the security fix branch instead of the master. We just need to ensure that the Security fix is in the master.

@aheritier @daniel-beck @KostyaSha Would be great to accelerate this PR, because there is a new LTS line on the horizon. If the change wants to get there, it needs to be soaked as much as possible. Or we can put the PR on hold and merge it when the new LTS gets selected

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Oct 24, 2016

Member

@oleg-nenashev (and @aheritier )

I do not see a reason to keep 2.7.x as well. Upgrade to 2.7.1 has been done from 2.5

That was back when pinning of plugins existed. Now, we just force everything to update at least to the bundled release.

Don't let me block you. I expect upset users who get force-fed updates, but maybe that fear is unnecessary.

Member

daniel-beck commented Oct 24, 2016

@oleg-nenashev (and @aheritier )

I do not see a reason to keep 2.7.x as well. Upgrade to 2.7.1 has been done from 2.5

That was back when pinning of plugins existed. Now, we just force everything to update at least to the bundled release.

Don't let me block you. I expect upset users who get force-fed updates, but maybe that fear is unnecessary.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 25, 2016

Member

I expect upset users who get force-fed updates, but maybe that fear is unnecessary.

Well, detached plugins are "special". I would definitely expect somebody to perform manual upgradeability tests before going forward. If it can be done in ATH, then it's even better

Member

oleg-nenashev commented Oct 25, 2016

I expect upset users who get force-fed updates, but maybe that fear is unnecessary.

Well, detached plugins are "special". I would definitely expect somebody to perform manual upgradeability tests before going forward. If it can be done in ATH, then it's even better

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 28, 2016

Member

Did some manual testing of upgrades for Jenkins instances with Maven Project plugin. Looks good so far

Member

oleg-nenashev commented Oct 28, 2016

Did some manual testing of upgrades for Jenkins instances with Maven Project plugin. Looks good so far

@aheritier

This comment has been minimized.

Show comment
Hide comment
@aheritier

aheritier Oct 28, 2016

Member

🐝 👍 @oleg-nenashev let's move forward

Member

aheritier commented Oct 28, 2016

🐝 👍 @oleg-nenashev let's move forward

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Oct 28, 2016

Member

@daniel-beck Are you fine with the PR since we've got some testing?

Member

oleg-nenashev commented Oct 28, 2016

@daniel-beck Are you fine with the PR since we've got some testing?

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Oct 28, 2016

Member

Don't let me block you.

Member

daniel-beck commented Oct 28, 2016

Don't let me block you.

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha
Member

KostyaSha commented Oct 28, 2016

:shipit:

@oleg-nenashev oleg-nenashev merged commit f64a223 into jenkinsci:master Oct 28, 2016

3 checks passed

Jenkins This pull request looks good
Details
Jenkins job PR-2568
continuous-integration/jenkins/pr-head This commit looks good
Details

oleg-nenashev added a commit that referenced this pull request Oct 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment