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

Stop storing three identical copies of remoting.jar in jenkins.war #2633

Merged
merged 1 commit into from Nov 19, 2016

Conversation

8 participants
@jglick
Member

jglick commented Nov 13, 2016

Should reduce the download size, and eliminate a source of confusion.

@reviewbybees

@recena

This comment has been minimized.

Show comment
Hide comment
@recena

recena Nov 13, 2016

Contributor

🐝 🐝 🐝 🐝 🐝 🐝 🐝 🐝

Contributor

recena commented Nov 13, 2016

🐝 🐝 🐝 🐝 🐝 🐝 🐝 🐝

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Nov 13, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 13, 2016

Member

Covered by Slave2Test.shouldNotEscapeJnlpSlavesResources BTW, and I also tested

curl -I http://localhost:8080/jenkins/jnlpJars/slave.jar

after both

mvn -f war hudson-dev:run

and

JENKINS_HOME=… java -jar war/target/jenkins.war
Member

jglick commented Nov 13, 2016

Covered by Slave2Test.shouldNotEscapeJnlpSlavesResources BTW, and I also tested

curl -I http://localhost:8080/jenkins/jnlpJars/slave.jar

after both

mvn -f war hudson-dev:run

and

JENKINS_HOME=… java -jar war/target/jenkins.war
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
Member

daniel-beck commented Nov 13, 2016

👍

@oleg-nenashev

🐛 for the binary compatibility.

I'm also not convinced this is a right way. I've seen cases when people repackage Jenkins.war and put their custom remoting implementations there. In such case the files may be actually different. So such change may break use-cases even if they are not that valid. Needs more votes from @jenkinsci/code-reviewers

@@ -373,7 +375,7 @@ private URLConnection connect() throws IOException {
return res.openConnection();
}
public URL getURL() throws MalformedURLException {
public URL getURL() throws IOException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

Not a compatible change since you switch to the upper level class 🐛

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

Not a compatible change since you switch to the upper level class 🐛

This comment has been minimized.

@jglick

jglick Nov 13, 2016

Member

Actually it is binary compatible, if not source compatible. Really this method is only public for tests and should have been restricted.

@jglick

jglick Nov 13, 2016

Member

Actually it is binary compatible, if not source compatible. Really this method is only public for tests and should have been restricted.

This comment has been minimized.

@daniel-beck

daniel-beck Nov 13, 2016

Member

@jglick Let's just do that now then.

@daniel-beck

daniel-beck Nov 13, 2016

Member

@jglick Let's just do that now then.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

Well, I'd bet that Slave#getURL is popular in plugins

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

Well, I'd bet that Slave#getURL is popular in plugins

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

@jglick It is binary compatible iff the WAR is exploded correctly. Custom WARs may really cause unhandled IOExceptions

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

@jglick It is binary compatible iff the WAR is exploded correctly. Custom WARs may really cause unhandled IOExceptions

This comment has been minimized.

@jglick

jglick Nov 14, 2016

Member

I'd bet that Slave#getURL is popular in plugins

This is Slave.JnlpJar.getURL. I cannot imagine many reasons for plugin code to be calling it. I have found seven calls to readFully, which calls getURL indirectly; and two direct calls, from methods which already threw IOException.

It is binary compatible iff the WAR is exploded correctly.

No, it is binary compatible always. An IOException thrown from a method not declared in source code to throw it is not considered a lack of binary compatibility. It will just be thrown up the call stack until someone handles it, just like a RuntimeException.

@jglick

jglick Nov 14, 2016

Member

I'd bet that Slave#getURL is popular in plugins

This is Slave.JnlpJar.getURL. I cannot imagine many reasons for plugin code to be calling it. I have found seven calls to readFully, which calls getURL indirectly; and two direct calls, from methods which already threw IOException.

It is binary compatible iff the WAR is exploded correctly.

No, it is binary compatible always. An IOException thrown from a method not declared in source code to throw it is not considered a lack of binary compatibility. It will just be thrown up the call stack until someone handles it, just like a RuntimeException.

This comment has been minimized.

@jglick

jglick Nov 17, 2016

Member

Both already throw IOException (or Exception).

@jglick

jglick Nov 17, 2016

Member

Both already throw IOException (or Exception).

@@ -383,6 +385,8 @@ public URL getURL() throws MalformedURLException {
if (name.equals("hudson-cli.jar")) {
name="jenkins-cli.jar";
} else if (name.equals("slave.jar") || name.equals("remoting.jar")) {
name = "lib/" + Which.jarFile(Channel.class).getName();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

I'm not sure this change is compatible since one may patch WAR file and put different implementations

@oleg-nenashev

oleg-nenashev Nov 13, 2016

Member

I'm not sure this change is compatible since one may patch WAR file and put different implementations

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 13, 2016

Member

Why would you keep different versions of Remoting for use from agents? Seems unlikely.

Member

jglick commented Nov 13, 2016

Why would you keep different versions of Remoting for use from agents? Seems unlikely.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Nov 13, 2016

Member

I agree with Jesse here, having different war files in a patched Jenkins has never been supported in any way and seems very unlikely. Just put a reverse proxy in front and move on.

Member

daniel-beck commented Nov 13, 2016

I agree with Jesse here, having different war files in a patched Jenkins has never been supported in any way and seems very unlikely. Just put a reverse proxy in front and move on.

@daniel-beck daniel-beck added needs-review and removed needs-fix labels Nov 14, 2016

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Nov 14, 2016

Member

🐝 👍 💯

Member

stephenc commented Nov 14, 2016

🐝 👍 💯

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 14, 2016

Member
  1. It should be on hold till the new LTS line gets selected
  2. I still keep 🐛 since there are real usages of the API (even if the both ones are safe) in plugin production and test code. Likely the change will break PCT runs against the core
Member

oleg-nenashev commented Nov 14, 2016

  1. It should be on hold till the new LTS line gets selected
  2. I still keep 🐛 since there are real usages of the API (even if the both ones are safe) in plugin production and test code. Likely the change will break PCT runs against the core
@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 17, 2016

Member

Likely the change will break PCT runs against the core

Which PCT runs did you have in mind? And why would anything break? No exception will actually be thrown unless the WAR file is broken.

Member

jglick commented Nov 17, 2016

Likely the change will break PCT runs against the core

Which PCT runs did you have in mind? And why would anything break? No exception will actually be thrown unless the WAR file is broken.

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 19, 2016

Member

It should be on hold till the new LTS line gets selected

Why? As core PRs go, this one seems to be on the safe side—a pretty simple packaging change that should not affect runtime behavior.

Member

jglick commented Nov 19, 2016

It should be on hold till the new LTS line gets selected

Why? As core PRs go, this one seems to be on the safe side—a pretty simple packaging change that should not affect runtime behavior.

I abstain

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 19, 2016

Member

Which PCT runs did you have in mind? And why would anything break? No exception will actually be thrown unless the WAR file is broken.

Tested PCT on the morning. It seems to work fine, hence I abstain and remove the bug

Member

oleg-nenashev commented Nov 19, 2016

Which PCT runs did you have in mind? And why would anything break? No exception will actually be thrown unless the WAR file is broken.

Tested PCT on the morning. It seems to work fine, hence I abstain and remove the bug

@daniel-beck daniel-beck merged commit 33d3899 into jenkinsci:master Nov 19, 2016

2 checks passed

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

oleg-nenashev added a commit that referenced this pull request Nov 20, 2016

@uschindler

This comment has been minimized.

Show comment
Hide comment
@uschindler

uschindler Nov 21, 2016

I just have to note that this packaging change really affected at least one user (myself, working for Apache Lucene and maintaining the randomized JVM test environment that found all those famous bugs in the Java 7 GA, Java 7u40 versions, and many more): I have some scripts to start the slaves in VirtualBOX machines. This scripts were more or less a command line slave starter with a shell script. Basically this shell script extracted the slave.jar from the WAR file and sent it to the VirtualBOX machine, which broke by this change.

I was able to fix this by using a curl -s $JENKINS/jnlpJars/slave.jar, but it was very hard to find that change in the official changelog, because the message only talks about "remoting.jar"...

I just have to note that this packaging change really affected at least one user (myself, working for Apache Lucene and maintaining the randomized JVM test environment that found all those famous bugs in the Java 7 GA, Java 7u40 versions, and many more): I have some scripts to start the slaves in VirtualBOX machines. This scripts were more or less a command line slave starter with a shell script. Basically this shell script extracted the slave.jar from the WAR file and sent it to the VirtualBOX machine, which broke by this change.

I was able to fix this by using a curl -s $JENKINS/jnlpJars/slave.jar, but it was very hard to find that change in the official changelog, because the message only talks about "remoting.jar"...

@jglick jglick deleted the jglick:remoting.jar branch Nov 22, 2016

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Dec 1, 2016

Member

Found a regression (not really the fault of core): JENKINS-40092

Member

jglick commented Dec 1, 2016

Found a regression (not really the fault of core): JENKINS-40092

@mfriedenhagen

This comment has been minimized.

Show comment
Hide comment
@mfriedenhagen

mfriedenhagen Apr 10, 2017

Member

Unfortunately this broke my setup as well, in the past I just did an scp from /var/cache/jenkins/war/WEB-INF/slave.jar to the remote SSH node to ensure the jar matches.

Member

mfriedenhagen commented Apr 10, 2017

Unfortunately this broke my setup as well, in the past I just did an scp from /var/cache/jenkins/war/WEB-INF/slave.jar to the remote SSH node to ensure the jar matches.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Apr 10, 2017

Member

@mfriedenhagen See my explanation in https://issues.jenkins-ci.org/browse/JENKINS-43425 . We do not consider war/ structure as whatever API, hence there is no compatibility requirement there. Maybe it should be explicitly documented somewhere.

Member

oleg-nenashev commented Apr 10, 2017

@mfriedenhagen See my explanation in https://issues.jenkins-ci.org/browse/JENKINS-43425 . We do not consider war/ structure as whatever API, hence there is no compatibility requirement there. Maybe it should be explicitly documented somewhere.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Apr 10, 2017

Member

@daniel-beck WDYT, does it worth mentioning in the upgrade guide? If yes, I will create a PR

Member

oleg-nenashev commented Apr 10, 2017

@daniel-beck WDYT, does it worth mentioning in the upgrade guide? If yes, I will create a PR

@mfriedenhagen

This comment has been minimized.

Show comment
Hide comment
@mfriedenhagen

mfriedenhagen Apr 10, 2017

Member

I am fine with deleting the duplicated jars but a hint in the upgrade guide would have been great 😁

Member

mfriedenhagen commented Apr 10, 2017

I am fine with deleting the duplicated jars but a hint in the upgrade guide would have been great 😁

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Apr 10, 2017

Member

OK, will write it

Member

oleg-nenashev commented Apr 10, 2017

OK, will write it

oleg-nenashev added a commit to oleg-nenashev/maven-plugin that referenced this pull request Nov 9, 2017

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