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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-54128] Avoid calling Run.getLogFile #40

Merged
merged 1 commit into from
Jun 15, 2019
Merged

[JENKINS-54128] Avoid calling Run.getLogFile #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 49 additions & 8 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.2</version>
<version>3.43</version>
<relativePath />
</parent>

<name>Groovy Postbuild</name>
<groupId>org.jvnet.hudson.plugins</groupId>
<artifactId>groovy-postbuild</artifactId>
<version>2.5-SNAPSHOT</version>
<packaging>hpi</packaging>
<url>http://wiki.jenkins-ci.org/display/JENKINS/Groovy+Postbuild+Plugin</url>
<url>https://wiki.jenkins.io/display/JENKINS/Groovy+Postbuild+Plugin</url>

<developers>
<developer>
Expand All @@ -27,17 +28,17 @@
<licenses>
<license>
<name>The MIT license</name>
<url>http://www.opensource.org/licenses/mit-license.php</url>
<url>https://opensource.org/licenses/MIT</url>
<distribution>repo</distribution>
</license>
</licenses>

<properties>
<jenkins.version>2.60.3</jenkins.version>
<jenkins.version>2.121.1</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see.
getLogReader() is already provided in 2.60.3 and there looks no need to upgrade the target version.
Why do you want this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to leave target versions as possible to allow users with older Jenkins to install newer versions.

Copy link
Member Author

@jglick jglick Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want this change?

So that functional tests can verify that it actually works. Plugin versions old enough to be compatible with the 2.60.x line do not include the JEP-210 changes which were the trigger for the issue.

allow users with older Jenkins to install newer versions

Up to you as maintainer. Stats suggest it is pointless as the great majority of users who have even updated to the currently newest plugin version are already on Jenkins 1.121.1 or newer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Looks good to me.

Notes:

<java.level>8</java.level>
<badge-plugin.version>1.2</badge-plugin.version>
<workflow-step-api-plugin.version>2.14</workflow-step-api-plugin.version>
<workflow-cps-plugin.version>2.45</workflow-cps-plugin.version>
<workflow-step-api-plugin.version>2.20</workflow-step-api-plugin.version>
<workflow-cps-plugin.version>2.69</workflow-cps-plugin.version>
</properties>

<scm>
Expand All @@ -50,7 +51,7 @@
<repositories>
<repository>
<id>repo.jenkins-ci.org</id>
<url>http://repo.jenkins-ci.org/public/</url>
<url>https://repo.jenkins-ci.org/public/</url>
</repository>
</repositories>

Expand Down Expand Up @@ -87,7 +88,13 @@
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-job</artifactId>
<version>2.10</version>
<version>2.32</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-basic-steps</artifactId>
<version>2.15</version>
<scope>test</scope>
</dependency>
<dependency>
Expand All @@ -97,4 +104,38 @@
<scope>test</scope>
</dependency>
</dependencies>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>${workflow-step-api-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-api</artifactId>
<version>2.33</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-support</artifactId>
<version>3.3</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>2.2.6</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.19</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<version>1.60</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is just a note, not a change request)
Groovy-postbuild depends directly and enormously on script-security, and should depend on script-security explicitly and declare the same version in tests.
It’s removed from dependencies when extracting badge-plugin.
I’ll review dependencies after merging this and #42 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #43.

</dependency>
</dependencies>
</dependencyManagement>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: To avoid RequireUpperBoundDeps:

[WARNING] Rule 4: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.jenkins-ci.plugins:structs:1.14 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins:structs:1.14
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:structs:1.17
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-step-api:2.20
    +-org.jenkins-ci.plugins:structs:1.18
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-basic-steps:2.15
    +-org.jenkins-ci.plugins:structs:1.17
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:junit:1.20
    +-org.jenkins-ci.plugins:structs:1.2
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins.workflow:workflow-step-api:2.14
      +-org.jenkins-ci.plugins:structs:1.5
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.30
      +-org.jenkins-ci.plugins:structs:1.14
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:scm-api:2.2.6
      +-org.jenkins-ci.plugins:structs:1.9
,
Require upper bound dependencies error for org.jenkins-ci.plugins:script-security:1.39 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:badge:1.2
    +-org.jenkins-ci.plugins:script-security:1.39
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins:matrix-project:1.12
    +-org.jenkins-ci.plugins:script-security:1.13
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins:script-security:1.58
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-support:3.3
      +-org.jenkins-ci.plugins:script-security:1.39
,
Require upper bound dependencies error for org.jenkins-ci.plugins.workflow:workflow-api:2.30 paths to dependency are:
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.30
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-job:2.32
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.32
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-basic-steps:2.15
    +-org.jenkins-ci.plugins.workflow:workflow-api:2.33
and
+-org.jvnet.hudson.plugins:groovy-postbuild:2.5-SNAPSHOT
  +-org.jenkins-ci.plugins.workflow:workflow-cps:2.69
    +-org.jenkins-ci.plugins.workflow:workflow-support:3.3
      +-org.jenkins-ci.plugins.workflow:workflow-api:2.30

It would be warned if versions in dependencyManagement is older than actual required ones, and this is enough safe even when updating dependency versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there looks artifacts not warned in RequireUpperBoundDeps.
And the versions looks newer than required ones.
With what policy do you specify these versions?

I want to know the best practice to avoid RequireUpperBoundDeps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With what policy do you specify these versions?

I simply put managed versions of dependencies as new as RequireUpperBoundDeps errors requested, until there no more such errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks.

</project>
32 changes: 26 additions & 6 deletions src/main/java/org/jvnet/hudson/plugins/groovypostbuild/GroovyPostbuildRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import groovy.lang.Binding;
import hudson.AbortException;
import hudson.EnvVars;
import hudson.Functions;
import hudson.Launcher;
import hudson.matrix.MatrixAggregatable;
import hudson.matrix.MatrixAggregator;
Expand Down Expand Up @@ -254,7 +255,7 @@ public void buildScriptFailed(Exception e) {

@Whitelisted
public boolean logContains(String regexp) {
return contains(build.getLogFile(), build.getCharset(), regexp);
return getLogMatcher(regexp) != null;
}


Expand All @@ -263,6 +264,7 @@ public boolean contains(File f, String regexp) {
return contains(f, Charset.defaultCharset(), regexp);
}

@Deprecated
// not @Whitelisted unless we know what file that is
public boolean contains(File f, Charset charset, String regexp) {
Matcher matcher = getMatcher(f, charset, regexp);
Expand All @@ -271,18 +273,23 @@ public boolean contains(File f, Charset charset, String regexp) {

@Whitelisted
public Matcher getLogMatcher(String regexp) {
return getMatcher(build.getLogFile(), build.getCharset(), regexp);
try (Reader r = build.getLogReader()) {
return getMatcher(r, regexp);
} catch (IOException e) {
Functions.printStackTrace(e, listener.error("Groovy Postbuild: logContains(\"" + regexp + "\") failed."));
buildScriptFailed(e);
return null;
}
}

@Deprecated
public Matcher getMatcher(File f, String regexp) {
return getMatcher(f, Charset.defaultCharset(), regexp);
}

public Matcher getMatcher(File f, Charset charset, String regexp) {
LOGGER.fine("Searching for '" + regexp + "' in '" + f + "'.");
public Matcher getMatcher(Reader r, String regexp) {
Matcher matcher = null;
try (BufferedReader reader = new BufferedReader(new InputStreamReader(new FileInputStream(f), charset))) {
try (BufferedReader reader = new BufferedReader(r)) {
Pattern pattern = compilePattern(regexp);
// Assume default encoding and text files
String line;
Expand All @@ -294,10 +301,23 @@ public Matcher getMatcher(File f, Charset charset, String regexp) {
}
}
} catch (IOException e) {
e.printStackTrace(listener.error("Groovy Postbuild: getMatcher(\"" + f + "\", \"" + regexp + "\") failed."));
Functions.printStackTrace(e, listener.error("Groovy Postbuild: getMatcher(…, \"" + regexp + "\") failed."));
buildScriptFailed(e);
}
return matcher;

}

@Deprecated
public Matcher getMatcher(File f, Charset charset, String regexp) {
LOGGER.fine("Searching for '" + regexp + "' in '" + f + "'.");
try (InputStream is = new FileInputStream(f); Reader r = new InputStreamReader(is, charset)) {
return getMatcher(r, regexp);
} catch (IOException e) {
Functions.printStackTrace(e, listener.error("Groovy Postbuild: getMatcher(\"" + f + "\", \"" + regexp + "\") failed."));
buildScriptFailed(e);
}
return null;
}

private Pattern compilePattern(String regexp) throws AbortException {
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/org/jvnet/hudson/plugins/groovypostbuild/WorkflowTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,18 @@
import org.jvnet.hudson.test.JenkinsRule;

import com.jenkinsci.plugins.badge.action.BadgeAction;
import java.util.Collections;
import java.util.logging.Level;
import org.jvnet.hudson.test.LoggerRule;

public class WorkflowTest {

@Rule
public JenkinsRule r = new JenkinsRule();

@Rule
public LoggerRule logging = new LoggerRule();

@Issue("JENKINS-26918")
@Test
public void usingManager() throws Exception {
Expand All @@ -48,4 +54,18 @@ public void usingManager() throws Exception {
assertEquals("stuff is broken", b.getAction(BadgeAction.class).getText());
}

@Issue("JENKINS-54128")
@Test
public void logContains() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"echo '1st message'\n" +
"echo '2nd message'\n" +
"sleep 1\n" + // to flush output (inspecting Run.log from the build itself is unreliable; use a TaskListenerDecorator instead)
"echo(/found first message? ${manager.logContains(/1st message/)} second? ${manager.logContains(/2nd message/)} third? ${manager.logContains(/3rd message/)} /); ", true));
logging.record(WorkflowRun.class, Level.WARNING).capture(100);
r.assertLogContains("found first message? true second? true third? false", r.assertBuildStatusSuccess(p.scheduleBuild2(0)));
assertEquals(Collections.emptyList(), logging.getRecords());
}

}