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

[JENKINS-26100] Change buildEnvVars to Run. #182

Merged
merged 7 commits into from Jun 16, 2017

Conversation

Projects
None yet
5 participants
@abayer
Copy link
Member

commented May 1, 2017

JENKINS-26100

NOTE: Findbugs is disabled due to the fact that there are 33 freakin'
findbugs warnings that cause the whole build to error out, and fixing
them is going to take a non-trivial amount of refactoring. I may well
come back to that in this PR, but it may need to wait for another PR.

cc @reviewbybees

[JENKINS-26100] Change buildEnvVars to Run.
NOTE: Findbugs is disabled due to the fact that there are 33 freakin'
findbugs warnings that cause the whole build to error out, and fixing
them is going to take a non-trivial amount of refactoring. I may well
come back to that in this PR, but it may need to wait for another PR.

@abayer abayer requested a review from jglick May 1, 2017

@jglick

jglick approved these changes May 1, 2017

Copy link
Member

left a comment

OK except for requested buildEnvVars rename.

pom.xml Outdated
<java.level>6</java.level>
<jenkins-core.version>2.58-20170501.171905-3</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<jenkins-war.version>2.58-20170501.171927-3</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<java.level>7</java.level>

This comment has been minimized.

Copy link
@jglick
@@ -666,12 +666,18 @@ public boolean isFilterChangelog() {
}

/**
* Sets the <tt>SVN_REVISION_n</tt> and <tt>SVN_URL_n</tt> environment variables during the build.
* Needed to make Mockito happy due to not picking up the parent implementation.

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

That is why you need a different name.

abayer added some commits May 2, 2017

pom.xml Outdated
<jenkins.version>1.609.3</jenkins.version>
<java.level>6</java.level>
<jenkins-core.version>2.58-20170502.192524-8</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<jenkins-war.version>2.58-20170502.192544-8</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->

This comment has been minimized.

Copy link
@jglick
@reviewbybees

This comment has been minimized.

Copy link

commented May 18, 2017

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.

@oleg-nenashev
Copy link
Member

left a comment

🐛 from what I see, core bump has been done to just add the nice @Override annotation. But you can remove annotation, and the code will probably work as expected on new cores without core bump

pom.xml Outdated
@@ -62,12 +62,13 @@ THE SOFTWARE.
</scm>

<properties>
<jenkins.version>1.609.3</jenkins.version>
<java.level>6</java.level>
<jenkins.version>2.60</jenkins.version>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Needs feedback from the plugin maintainer. I am not comfortable with pushing master version and dropping support of all LTS versions just to get the improvement landed

* Sets the <tt>SVN_REVISION_n</tt> and <tt>SVN_URL_n</tt> environment variables during the build.
*/
@Override
public void buildEnvironment(Run<?, ?> build, Map<String, String> env) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Why don't you just create this method without override definition? In such case you won't need to bump the core, right?

This comment has been minimized.

Copy link
@abayer

abayer May 18, 2017

Author Member

I honestly don't know - I assumed it wouldn't work right if it wasn't explicitly overriding.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

IIRC we were doing it many times to pick new APIs in the core without bumping the dependency. CC @jglick to double check

This comment has been minimized.

Copy link
@abayer

abayer May 18, 2017

Author Member

Well, one problem is that we'd also have to add a buildEnvVars(AbstractBuild,Map) method to the SCMs, since they wouldn't be able to rely on the updated SCM.buildEnvVars(AbstractBuild,Map) method...

This comment has been minimized.

Copy link
@abayer

abayer May 18, 2017

Author Member

Ok, a quick test with git seems to suggest this will work, so switching around.

pom.xml Outdated
<no-test-jar>false</no-test-jar>
<workflow.version>1.14.2</workflow.version>
<findbugs.failOnError>false</findbugs.failOnError>
<scm-api-plugin.version>2.0.8</scm-api-plugin.version>
<findbugs.skip>true</findbugs.skip> <!-- TODO: Fix the large number of findbugs warnings -->

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

🐛 It it is supposed to hurt. Nobody will fix them otherwise

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 18, 2017

Switched back to 1.609.3 baseline etc - but note that there are still tons of findbugs errors. I don't know if those were present beforehand or what.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I don't know if those were present beforehand or what.

They were, no worries

public void buildEnvVars(AbstractBuild<?, ?> build, Map<String, String> env) {
super.buildEnvVars(build, env);

public void buildEnvironment(Run<?, ?> build, Map<String, String> env) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

I would add info about target Jenkins core version to Javadoc. maybe with "TODO: override when the baseline is above N"

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member
// TODO 2.60+ add @Override
@@ -231,7 +231,7 @@ protected void retrieve(@CheckForNull SCMSourceCriteria criteria,
@NonNull final SCMHeadObserver observer,
@CheckForNull SCMHeadEvent<?> event,
@NonNull TaskListener listener)
throws IOException {
throws IOException, InterruptedException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

IMHO not required. I also doubt it is binary compatible for methods, which explicitly call SubversionScmSource instead of the method on the parent class.

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member

If the body does not throw it then it may be removed. It is not a binary compatibility issue since the JVM does not care about checked exceptions.

@jglick

jglick approved these changes May 18, 2017

Copy link
Member

left a comment

Indeed you can be considered to override a method even without the annotation, and even if your class was compiled against a version of the supertype which did not define that method.

@@ -665,13 +665,15 @@ public boolean isFilterChangelog() {
return filterChangelog;
}

@Override

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member
// TODO 2.60+ delete this overload
@@ -231,7 +231,7 @@ protected void retrieve(@CheckForNull SCMSourceCriteria criteria,
@NonNull final SCMHeadObserver observer,
@CheckForNull SCMHeadEvent<?> event,
@NonNull TaskListener listener)
throws IOException {
throws IOException, InterruptedException {

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member

If the body does not throw it then it may be removed. It is not a binary compatibility issue since the JVM does not care about checked exceptions.

@@ -347,7 +347,7 @@ void fetch(@NonNull TaskListener listener,
@NonNull SortedSet<List<String>> excludedPaths,
@CheckForNull SCMSourceCriteria branchCriteria,
@NonNull SCMHeadObserver observer)
throws IOException, SVNException {
throws IOException, SVNException, InterruptedException {

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member

Ditto.

This comment has been minimized.

Copy link
@abayer

abayer May 19, 2017

Author Member

Done.

@@ -108,6 +109,7 @@ public void shouldSetEnvironmentVariablesWithMultipleSvnModules() throws IOExcep
private SubversionSCM mockSCMForBuildEnvVars() {
SubversionSCM scm = mock(SubversionSCM.class);
doCallRealMethod().when(scm).buildEnvVars(any(AbstractBuild.class), anyMapOf(String.class, String.class));
doCallRealMethod().when(scm).buildEnvironment(any(Run.class), anyMapOf(String.class, String.class));

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member

So does this allow

mvn -Djenkins.version=2.60 clean test

to work? What about PCT’s

mvn clean test-compile && mvn -Djenkins.version=2.60 surefire:test

?

This comment has been minimized.

Copy link
@abayer

abayer May 19, 2017

Author Member

Yup, works.

public void buildEnvVars(AbstractBuild<?, ?> build, Map<String, String> env) {
super.buildEnvVars(build, env);

public void buildEnvironment(Run<?, ?> build, Map<String, String> env) {

This comment has been minimized.

Copy link
@jglick

jglick May 18, 2017

Member
// TODO 2.60+ add @Override
@jglick

jglick approved these changes May 19, 2017

@oleg-nenashev
Copy link
Member

left a comment

🐝

@abayer

This comment has been minimized.

Copy link
Member Author

commented May 20, 2017

@abayer abayer merged commit 0613ef3 into jenkinsci:master Jun 16, 2017

1 check passed

Jenkins This pull request looks good
Details
@jglick

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

Do you need a release cut of this?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

The plugin has no maintainer anymore, so yeah, whomever takes it first

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2017

@jglick Yup, a release would be handy. =)

@James-Dengel

This comment has been minimized.

Copy link

commented Aug 10, 2017

@jglick Yes that would be great as I'm looking at this right now.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

@James-Dengel It has been already released in Subversion 2.8 from what I see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.