Skip to content

[JENKINS-60245] Potential fix for @Library annotation in load()'d scripts #87

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

Open
awittha wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JENKINS-60245] Potential fix for @Library annotation in load()'d scripts #87

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
13 changes: 11 additions & 2 deletions src/main/java/org/jenkinsci/plugins/workflow/libs/ClasspathAdder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@
import groovy.lang.GroovyShell;
import hudson.ExtensionPoint;
import java.net.URL;
import java.util.List;
import java.util.HashMap;
import java.util.List;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.jenkinsci.plugins.workflow.cps.CpsFlowExecution;

/**
Expand Down Expand Up @@ -57,13 +58,21 @@ void addTo(@Nonnull CpsFlowExecution execution) {

}

/**
* @see #add(String, CpsFlowExecution, List, HashMap)
*/
public @Nonnull List<Addition> add(@Nonnull CpsFlowExecution execution, @Nonnull List<String> libraries, @Nonnull HashMap<String, Boolean> changelogs) throws Exception {
return add( null, execution, libraries, changelogs );
}

/**
* May add to the classpath.
* @param scope identifier for the source file that the addition came from
* @param execution a running build (possibly newly started, possibly resumed)
* @param libraries aggregated entries from all encountered {@link Library#value} (will be empty if {@link Library} is never used at all); an implementation should remove entries it “claims”
* @return a possibly empty list of additions
* @throws Exception for whatever reason (will fail compilation)
*/
public abstract @Nonnull List<Addition> add(@Nonnull CpsFlowExecution execution, @Nonnull List<String> libraries, @Nonnull HashMap<String, Boolean> changelogs) throws Exception;
public abstract @Nonnull List<Addition> add(@Nullable String scope, @Nonnull CpsFlowExecution execution, @Nonnull List<String> libraries, @Nonnull HashMap<String, Boolean> changelogs) throws Exception;

@awittha awittha Nov 21, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

New add(...) method added to the ClasspathAdder interface, to allow for scoping the source of an addition.


}
16 changes: 15 additions & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/libs/LibrariesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,15 @@
class LibrariesAction extends InvisibleAction {

private final List<LibraryRecord> libraries;


private final String scope;

LibrariesAction(List<LibraryRecord> libraries) {
this(null, libraries);
}

LibrariesAction(String scope, List<LibraryRecord> libraries) {
this.scope = scope;

@awittha awittha Nov 21, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

New constructor & field added to LibrariesAction to allow recording of "scope" - what source file the libraries in this LibrariesAction are from.

this.libraries = libraries;
}

Expand All @@ -50,6 +57,13 @@ class LibrariesAction extends InvisibleAction {
public List<LibraryRecord> getLibraries() {
return libraries;
}

/**
* @return An identifier of the source file that these library definitions are for
*/
public String getScope() {
return scope;
}

@Extension public static class LibraryEnvironment extends EnvironmentContributor {

Expand Down
56 changes: 34 additions & 22 deletions src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@

private static final Logger LOGGER = Logger.getLogger(LibraryAdder.class.getName());

@Override public List<Addition> add(CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
@Override public List<Addition> add(String scope, CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
Queue.Executable executable = execution.getOwner().getExecutable();
Run<?,?> build;
if (executable instanceof Run) {
Expand All @@ -85,7 +85,19 @@
librariesUnparsed.put(parsed[0], library);
}
List<Addition> additions = new ArrayList<>();
LibrariesAction action = build.getAction(LibrariesAction.class);
LibrariesAction action = null;

for( LibrariesAction laction : build.getActions( LibrariesAction.class ) ) {
if( laction.getScope() != null && laction.getScope().equals( scope ) ) {

@awittha awittha Nov 21, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Previously, as there was only ever one LibrariesAction per build, if it was present, it was concluded that this was a resumed build and that action could be used.

Now, there may be multiple actions so we must find the one whose scope equals the scope passed into this add invocation.

// found the action that's absolutely right for this scope
action = laction;
break;
} else if( laction.getScope() == null ) {
// generic LibrariesAction may be overridden later if there's a perfect match.
action = laction;

@awittha awittha Nov 21, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is possible that a build was suspended before these code changes landed, in which case the "right" LibrariesAction will have a null scope.

}
}

if (action != null) {
// Resuming a build, so just look up what we loaded before.
for (LibraryRecord record : action.getLibraries()) {
Expand Down Expand Up @@ -131,12 +143,14 @@
}
}
// Record libraries we plan to load. We need LibrariesAction there first so variables can be interpolated.
build.addAction(new LibrariesAction(new ArrayList<>(librariesAdded.values())));
// Now actually try to retrieve the libraries.
for (LibraryRecord record : librariesAdded.values()) {
listener.getLogger().println("Loading library " + record.name + "@" + record.version);
for (URL u : retrieve(record.name, record.version, retrievers.get(record.name), record.trusted, record.changelog, listener, build, execution, record.variables)) {
additions.add(new Addition(u, record.trusted));
if( librariesAdded.size() > 0 ) {
build.addAction(new LibrariesAction(new ArrayList<>(librariesAdded.values())));

@awittha awittha Nov 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Avoid adding a LibrariesAction with 0 library definitions in it.

This feels more right to me - I couldn't figure any solid reason for adding a LibrariesAction with no definitions. Is there one? I am not married to this particular semantic change; it was just instinct to not add a record of nothing.

// Now actually try to retrieve the libraries.
for (LibraryRecord record : librariesAdded.values()) {
listener.getLogger().println("Loading library " + record.name + "@" + record.version);
for (URL u : retrieve(record.name, record.version, retrievers.get(record.name), record.trusted, record.changelog, listener, build, execution, record.variables)) {
additions.add(new Addition(u, record.trusted));
}
}
}
return additions;
Expand Down Expand Up @@ -200,8 +214,7 @@ static List<URL> retrieve(@Nonnull String name, @Nonnull String version, @Nonnul
Queue.Executable executable = execution.getOwner().getExecutable();
if (executable instanceof Run) {
Run<?,?> run = (Run) executable;
LibrariesAction action = run.getAction(LibrariesAction.class);
if (action != null) {
for( LibrariesAction action : run.getActions(LibrariesAction.class)) {
FilePath libs = new FilePath(run.getRootDir()).child("libs");
for (LibraryRecord library : action.getLibraries()) {
FilePath f = libs.child(library.name + "/resources/" + name);
Expand Down Expand Up @@ -230,16 +243,17 @@ private static String readResource(FilePath file, @CheckForNull String encoding)
if (run == null) {
return Collections.emptySet();
}
LibrariesAction action = run.getAction(LibrariesAction.class);
if (action == null) {
return Collections.emptySet();
}

List<GlobalVariable> vars = new ArrayList<>();
for (LibraryRecord library : action.getLibraries()) {
for (String variable : library.variables) {
vars.add(new UserDefinedGlobalVariable(variable, new File(run.getRootDir(), "libs/" + library.name + "/vars/" + variable + ".txt")));

for( LibrariesAction action : run.getActions(LibrariesAction.class)) {
for (LibraryRecord library : action.getLibraries()) {
for (String variable : library.variables) {
vars.add(new UserDefinedGlobalVariable(variable, new File(run.getRootDir(), "libs/" + library.name + "/vars/" + variable + ".txt")));
}
}
}

return vars;
}

Expand All @@ -255,8 +269,7 @@ private static String readResource(FilePath file, @CheckForNull String encoding)
Queue.Executable executable = execution.getOwner().getExecutable();
if (executable instanceof Run) {
Run<?,?> run = (Run) executable;
LibrariesAction action = run.getAction(LibrariesAction.class);
if (action != null) {
for( LibrariesAction action : run.getActions(LibrariesAction.class)) {
FilePath libs = new FilePath(run.getRootDir()).child("libs");
for (LibraryRecord library : action.getLibraries()) {
if (library.trusted) {
Expand All @@ -273,7 +286,7 @@ private static String readResource(FilePath file, @CheckForNull String encoding)
}
}
}
}
}
}
} catch (IOException | InterruptedException x) {
LOGGER.log(Level.WARNING, null, x);
Expand All @@ -286,8 +299,7 @@ private static String readResource(FilePath file, @CheckForNull String encoding)
@Extension public static class Copier extends FlowCopier.ByRun {

@Override public void copy(Run<?,?> original, Run<?,?> copy, TaskListener listener) throws IOException, InterruptedException {
LibrariesAction action = original.getAction(LibrariesAction.class);
if (action != null) {
for( LibrariesAction action : original.getActions(LibrariesAction.class )) {
copy.addAction(new LibrariesAction(action.getLibraries()));
FilePath libs = new FilePath(original.getRootDir()).child("libs");
if (libs.isDirectory()) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ private void processExpression(SourceUnit source, List<String> libraries, Expres
}.visitClass(classNode);
try {
for (ClasspathAdder adder : ExtensionList.lookup(ClasspathAdder.class)) {
for (ClasspathAdder.Addition addition : adder.add(execution, libraries, changelogs)) {
for (ClasspathAdder.Addition addition : adder.add(source.getName(), execution, libraries, changelogs)) {
addition.addTo(execution);
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,15 @@ public static class Execution extends AbstractSynchronousNonBlockingStepExecutio
}

LibraryRecord record = new LibraryRecord(name, version, trusted, changelog);
LibrariesAction action = run.getAction(LibrariesAction.class);
LibrariesAction action = null;

for( LibrariesAction laction : run.getActions(LibrariesAction.class)) {
if( null == laction.getScope() ) {
action = laction;
break;
}
}

if (action == null) {
action = new LibrariesAction(Lists.newArrayList(record));
run.addAction(action);
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryDecoratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public class LibraryDecoratorTest {
}

@TestExtension public static class TestAdder extends ClasspathAdder {
@Override public List<Addition> add(CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
@Override public List<Addition> add(String scope, CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
List<Addition> additions = new ArrayList<>();
for (String library : libraries) {
additions.add(new Addition(new File(((WorkflowRun) execution.getOwner().getExecutable()).getParent().getRootDir(), "libs/" + library).toURI().toURL(), false));
Expand Down Expand Up @@ -101,7 +101,7 @@ public class LibraryDecoratorTest {
r.assertLogContains("failed to load [stuff]", r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0)));
}
@TestExtension("adderError") public static class ErroneousAdder extends ClasspathAdder {
@Override public List<Addition> add(CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
@Override public List<Addition> add(String scope, CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
throw new AbortException("failed to load " + libraries);
}
}
Expand Down