Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
11 changes: 7 additions & 4 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpcore</artifactId>
</exclusion>
<exclusion>
<groupId>org.apache.ant</groupId>
<artifactId>ant</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
Expand Down Expand Up @@ -151,10 +155,9 @@
<scope>test</scope>
</dependency>
<dependency>
<!-- Required by maven-plugin -->
<groupId>org.apache.ant</groupId>
<artifactId>ant</artifactId>
<version>1.9.2</version>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>compress-artifacts</artifactId>
<version>1.10</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/hudson/plugins/copyartifact/BuildSelector.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import hudson.model.Run;
import java.io.IOException;
import java.io.PrintStream;
import javax.annotation.CheckForNull;
import jenkins.util.VirtualFile;

/**
* Extension point for selecting the build to copy artifacts from.
Expand Down Expand Up @@ -112,6 +114,26 @@ protected static boolean isBuildResultBetterOrEqualTo(Run<?,?> run, Result resul
return buildResult.isBetterOrEqualTo(resultToTest);
}

/**
* Load artifacts from a given build.
* @param sourceBuild a build which may have associated artifacts
* @param console a way to print messages
* @return a {@linkplain VirtualFile#isDirectory directory} of artifacts, or null if missing
*/
protected @CheckForNull VirtualFile getArtifacts(Run<?, ?> sourceBuild, PrintStream console) throws IOException, InterruptedException {
if (Util.isOverridden(BuildSelector.class, getClass(), "getSourceDirectory", Run.class, PrintStream.class)) {
FilePath old = getSourceDirectory(sourceBuild, console);
return old != null ? old.toVirtualFile() : null;
} else {
VirtualFile root = sourceBuild.getArtifactManager().root();
return root.isDirectory() ? root : null;
}
}

/**
* @deprecated rather override {@link #getArtifacts}
*/
@Deprecated
protected FilePath getSourceDirectory(Run<?,?> src, PrintStream console) throws IOException, InterruptedException {
FilePath srcDir = new FilePath(src.getArtifactsDir());
if (srcDir.exists()) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/hudson/plugins/copyartifact/Copier.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@
* @author Alan Harder
* @author Kohsuke Kawaguchi
* @see "JENKINS-7753"
* @deprecated No longer used.
*/
@Deprecated
public abstract class Copier implements ExtensionPoint {
Copy link
Member

Choose a reason for hiding this comment

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

We can remove them.
It's useless to preserving binary compatibilities as they're completely no longer used,
and exceptions should occur if someone use them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, no “exception” would result from using them somehow. I do not have a strong opinion either way.


private static Logger LOG = Logger.getLogger(Copier.class.getName());
Expand Down
138 changes: 114 additions & 24 deletions src/main/java/hudson/plugins/copyartifact/CopyArtifact.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,28 @@
import hudson.security.SecurityRealm;
import hudson.tasks.BuildStepDescriptor;
import hudson.tasks.Builder;
import hudson.tasks.Fingerprinter;
import hudson.util.DescribableList;
import hudson.util.FormValidation;
import hudson.util.VariableResolver;
import hudson.util.XStream2;

import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintStream;
import java.security.DigestOutputStream;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;

import jenkins.model.Jenkins;

Expand All @@ -78,6 +86,12 @@

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import jenkins.util.VirtualFile;
import org.apache.commons.io.IOUtils;
import org.apache.tools.ant.DirectoryScanner;
import org.apache.tools.ant.types.selectors.SelectorUtils;
import org.apache.tools.ant.types.selectors.TokenizedPath;
import org.apache.tools.ant.types.selectors.TokenizedPattern;

/**
* Build step to copy artifacts from another project.
Expand Down Expand Up @@ -412,7 +426,7 @@ public void perform(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonn
throw new AbortException(message);
}
}
FilePath targetDir = workspace, baseTargetDir = targetDir;
Copy link
Member

Choose a reason for hiding this comment

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

Note for me: baseTargerDir looks not used at all. Completely useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, AFAICT it was never used anyway.

FilePath targetDir = workspace;
targetDir.mkdirs(); // being a SimpleBuildStep guarantees it will have a workspace, but the physical dir might not yet exist.
// Add info about the selected build into the environment
EnvAction envData = build.getAction(EnvAction.class);
Expand All @@ -429,18 +443,16 @@ public void perform(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonn
expandedExcludes = null;
}

Copier copier = jenkins.getExtensionList(Copier.class).get(0).clone();

if (jenkins.getPlugin("maven-plugin") != null && (src instanceof MavenModuleSetBuild) ) {
// use classes in the "maven-plugin" plugin as might not be installed
// Copy artifacts from the build (ArchiveArtifacts build step)
boolean ok = perform(src, build, expandedFilter, expandedExcludes, targetDir, baseTargetDir, copier, console);
boolean ok = perform(src, build, expandedFilter, expandedExcludes, targetDir, console);
// Copy artifacts from all modules of this Maven build (automatic archiving)
for (Iterator<MavenBuild> it = ((MavenModuleSetBuild)src).getModuleLastBuilds().values().iterator(); it.hasNext(); ) {
// for(Run r: ....values()) causes upcasting and loading MavenBuild compiled with jdk 1.6.
// SEE https://wiki.jenkins-ci.org/display/JENKINS/Tips+for+optional+dependencies for details.
Run<?,?> r = it.next();
ok |= perform(r, build, expandedFilter, expandedExcludes, targetDir, baseTargetDir, copier, console);
ok |= perform(r, build, expandedFilter, expandedExcludes, targetDir, console);
}
if (!ok) {
throw new AbortException(Messages.CopyArtifact_FailedToCopy(expandedProject, expandedFilter));
Expand All @@ -451,14 +463,13 @@ public void perform(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonn
// Use MatrixBuild.getExactRuns if available
for (Run r : ((MatrixBuild) src).getExactRuns())
// Use subdir of targetDir with configuration name (like "jdk=java6u20")
ok |= perform(r, build, expandedFilter, expandedExcludes, targetDir.child(r.getParent().getName()),
baseTargetDir, copier, console);
ok |= perform(r, build, expandedFilter, expandedExcludes, targetDir.child(r.getParent().getName()), console);

if (!ok) {
throw new AbortException(Messages.CopyArtifact_FailedToCopy(expandedProject, expandedFilter));
}
} else {
if (!perform(src, build, expandedFilter, expandedExcludes, targetDir, baseTargetDir, copier, console)) {
if (!perform(src, build, expandedFilter, expandedExcludes, targetDir, console)) {
throw new AbortException(Messages.CopyArtifact_FailedToCopy(expandedProject, expandedFilter));
}
}
Expand Down Expand Up @@ -512,33 +523,112 @@ private static ItemGroup getItemGroup(Run<?, ?> build) {
}


private boolean perform(Run src, Run<?,?> dst, String expandedFilter, String expandedExcludes, FilePath targetDir,
FilePath baseTargetDir, Copier copier, PrintStream console)
throws IOException, InterruptedException {
FilePath srcDir = selector.getSourceDirectory(src, console);
private boolean perform(Run src, Run<?,?> dst, String expandedFilter, @CheckForNull String expandedExcludes, FilePath targetDir, PrintStream console) throws IOException, InterruptedException {
VirtualFile srcDir = selector.getArtifacts(src, console);
if (srcDir == null) {
return isOptional(); // Fail build unless copy is optional
}

copier.initialize(src, dst, srcDir, baseTargetDir);
Map<String, String> fingerprints;
MessageDigest md5;
if (isFingerprintArtifacts()) {
fingerprints = new HashMap<>();
try {
md5 = MessageDigest.getInstance("MD5");
} catch (NoSuchAlgorithmException x) {
throw new AssertionError(x);
Copy link
Member

Choose a reason for hiding this comment

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

does not really matter, but maybe just IOException? There were some discussions about disabling MD5 at all to harden the security. I'd guess it is not realistic, but I would rather prefer to avoid errors in the production code

Copy link
Member Author

Choose a reason for hiding this comment

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

To do that we would first need to reimplement fingerprints in Jenkins core, which would surely force changes here as well anyway.

}
} else {
fingerprints = null;
md5 = null;
}
try {
int cnt;
if (!isFlatten())
cnt = copier.copyAll(srcDir, expandedFilter, expandedExcludes, targetDir, isFingerprintArtifacts());
else {
targetDir.mkdirs(); // Create target if needed
FilePath[] list = srcDir.list(expandedFilter, expandedExcludes, false);
for (FilePath file : list)
copier.copyOne(file, new FilePath(targetDir, file.getName()), isFingerprintArtifacts());
cnt = list.length;
targetDir.mkdirs(); // Create target if needed
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to switch it to Java7 Files API while you are around

List<String> list = scan(srcDir, expandedFilter, expandedExcludes);
for (String file : list) {
copyOne(src, dst, fingerprints, srcDir.child(file), new FilePath(targetDir, isFlatten() ? file.replaceFirst(".+/", "") : file), md5);
Copy link
Member

Choose a reason for hiding this comment

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

passing isFingerprintArtifacts() looked better, but OK

Copy link
Member Author

Choose a reason for hiding this comment

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

But now we need to pass in the map.

Copy link
Member

Choose a reason for hiding this comment

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

Note for me: snipping ".+/" is equivalent to the current implementation calling FilePath#getName() in FingerprintingCopyMethod.
(I'm not sure this is a good behavior as reported in JENKINS-16987)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this replaceFirst call just implements the flatten mode. It has nothing to do with fingerprinting. The behavior you mention is retained in copyOne as

fingerprints.put(s.getName(), digest);
//                ^^^^^^^^^^

Copy link
Member

Choose a reason for hiding this comment

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

}

int cnt = list.size();
console.println(Messages.CopyArtifact_Copied(cnt, HyperlinkNote.encodeTo('/'+ src.getParent().getUrl(), src.getParent().getFullDisplayName()),
HyperlinkNote.encodeTo('/'+src.getUrl(), Integer.toString(src.getNumber()))));
// Fail build if 0 files copied unless copy is optional
return cnt > 0 || isOptional();
} finally {
copier.end();
if (fingerprints != null) {
for (Run<?, ?> r : new Run<?, ?>[] {src, dst}) {
if (fingerprints.size() > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

It is not modified inside the cycle IIUC, maybe makes sense to just move to the outer check

Copy link
Member Author

Choose a reason for hiding this comment

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

Does not really matter—the loop has only two iterations.

Fingerprinter.FingerprintAction fa = r.getAction(Fingerprinter.FingerprintAction.class);
if (fa != null) {
fa.add(fingerprints);
} else {
r.addAction(new Fingerprinter.FingerprintAction(r, fingerprints));
}
}
}
}
}
}

private static List<String> scan(VirtualFile root, String expandedFilter, @CheckForNull String expandedExcludes) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be moved to core utils eventually. Looks to be generic enough

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is deleted in #100.

List<String> r = new ArrayList<>();
// TODO need VirtualFile.list(String, String, boolean) like FilePath offers
List<TokenizedPattern> patts = new ArrayList<>();
if (expandedExcludes != null) {
for (String patt : expandedExcludes.split(",")) {
patts.add(new TokenizedPattern(normalizePattern(patt)));
}
}
FILE: for (String child : root.list(expandedFilter)) {
child = child.replace('\\', '/'); // TODO list(String) ought to specify `/` as the separator
for (TokenizedPattern patt : patts) {
if (patt.matchPath(new TokenizedPath(child), true)) {
continue FILE;
}
}
r.add(child);
}
return r;
}

/** Similar to a method in {@link DirectoryScanner}. */
private static String normalizePattern(String p) {
String pattern = p.replace('\\', '/'); // we only deal with forward slashes here
if (pattern.endsWith("/")) {
pattern += SelectorUtils.DEEP_TREE_MATCH;
}
return pattern;
}

private static void copyOne(Run<?,?> src, Run<?,?> dst, Map<String, String> fingerprints, VirtualFile s, FilePath d, MessageDigest md5) throws IOException, InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

same, does not seem to be plugin-specific

Copy link
Member Author

Choose a reason for hiding this comment

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

It is pretty specific to copyartifact I think.

assert (fingerprints == null) == (md5 == null);
// TODO JENKINS-26810 handle symlinks and file attributes if supported
try {
try (InputStream is = s.open(); OutputStream os = d.write()) {
OutputStream os2;
if (md5 != null) {
md5.reset();
os2 = new DigestOutputStream(os, md5);
} else {
os2 = os;
}
IOUtils.copy(is, os2);
}
// FilePath.setLastModifiedIfPossible private; copyToWithPermission OK but would have to calc digest separately:
try {
d.touch(s.lastModified());
} catch (IOException x) {
LOGGER.warning(x.getMessage());
}
if (fingerprints != null) {
String digest = Util.toHexString(md5.digest());
FingerprintMap map = Jenkins.getActiveInstance().getFingerprintMap();
Fingerprint f = map.getOrCreate(src, s.getName(), digest);
f.addFor(src);
f.addFor(dst);
fingerprints.put(s.getName(), digest);
}
} catch (IOException e) {
throw new IOException("Failed to copy " + s + " to " + d, e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
* using the Jenkins FilePath class. Has -100 ordinal value so any other
* plugin implementing this extension point should override this one.
* @author Alan Harder
* @deprecated No longer used.
*/
@Deprecated
@Extension(ordinal=-200)
public class FilePathCopyMethod extends Copier {
/** @see FilePath#copyRecursiveTo(String,FilePath) */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@
* masks the cost of digest computation.
*
* @author Kohsuke Kawaguchi
* @deprecated No longer used.
*/
@Deprecated
@Extension(ordinal=-100)
public class FingerprintingCopyMethod extends Copier {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@
import java.io.PrintStream;

import jenkins.model.Jenkins;
import jenkins.util.VirtualFile;
import org.jenkinsci.Symbol;
import org.jvnet.localizer.Localizable;
import org.kohsuke.stapler.DataBoundConstructor;

/**
Expand All @@ -52,17 +52,17 @@ public boolean isSelectable(Run<?,?> run, EnvVars env) {
return true;
}

@Override protected FilePath getSourceDirectory(Run<?,?> src, PrintStream console) throws IOException, InterruptedException {
@Override protected VirtualFile getArtifacts(Run<?,?> src, PrintStream console) throws IOException, InterruptedException {
if (src instanceof AbstractBuild) {
FilePath srcDir = ((AbstractBuild) src).getWorkspace();
if (srcDir != null && srcDir.exists()) {
return srcDir;
return srcDir.toVirtualFile();
} else {
console.println(Messages.CopyArtifact_MissingSrcWorkspace()); // (see JENKINS-3330)
return null;
}
} else {
return super.getSourceDirectory(src, console);
return super.getArtifacts(src, console);
}
}

Expand Down
Loading