[JENKINS-60245] Potential fix for @Library annotation in load()'d scripts #87
Conversation
| * @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; |
There was a problem hiding this comment.
New add(...) method added to the ClasspathAdder interface, to allow for scoping the source of an addition.
| } | ||
|
|
||
| LibrariesAction(String scope, List<LibraryRecord> libraries) { | ||
| this.scope = scope; |
There was a problem hiding this comment.
New constructor & field added to LibrariesAction to allow recording of "scope" - what source file the libraries in this LibrariesAction are from.
| LibrariesAction action = null; | ||
|
|
||
| for( LibrariesAction laction : build.getActions( LibrariesAction.class ) ) { | ||
| if( laction.getScope() != null && laction.getScope().equals( scope ) ) { |
There was a problem hiding this comment.
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.
| break; | ||
| } else if( laction.getScope() == null ) { | ||
| // generic LibrariesAction may be overridden later if there's a perfect match. | ||
| action = laction; |
There was a problem hiding this comment.
It is possible that a build was suspended before these code changes landed, in which case the "right" LibrariesAction will have a null scope.
| 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()))); |
There was a problem hiding this comment.
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.
|
Thanks for the PR! I'm not totally sure about this, here are a few of the immediate questions that came to mind:
|
|
As of #172 most of the code in this plugin has been moved to another plugin repository so this PR must be closed. If this change is still needed, please git clone https://github.com/jenkinsci/pipeline-groovy-lib-plugin
cd pipeline-groovy-lib-plugin
git checkout -b JENKINS-60245
git pull https://github.com/awittha/workflow-cps-global-lib-plugin JENKINS-60245resolve any merge conflicts, and file a fresh PR on the new repository. Be sure to paste a link to this old PR to enable bidirectional navigation. |
This is a potential fix for the issue described in JENKINS-60245. I call it a "potential" fix because while it seems to fix all the broken use-cases and cause no regressions, I am not sure if there is a more architecturally-sound means to accomplish the same ends.
Please see the JIRA issue for a complete description of the problem.
These changes add a
String scopetoLibrariesAction, and attach one action to a build per loaded script.When recording shared libraries that were mentioned in a
@Libraryannotation, the script being processed is used as the scope of theLibrariesAction. This enables the plugin to know that it should index the contents of@Libraryannotations in newly-loaded scripts.When processing libraries to actually add their resources to the classpath, every
LibrariesActionfor the build is iterated over.This allows scripts loaded with the
load(...)step to add shared libraries with the@Library(...)annotation.I had to update some existing unit tests, but did not write a regression test for the new behavior. If this implementation of a solution to JENKINS-60245 is determined to be generally-sound & on-track for merging into the plugin, I will write such a test.