Skip to content

Commit

Permalink
[FIXED JENKINS-17889] added P4ONECHANGELIST property to allow sequent…
Browse files Browse the repository at this point in the history 
…ial building of changes
  • Loading branch information
@rpetti
rpetti committed Nov 20, 2013
1 parent ae5b010 commit f5c430c
Showing 1 changed file with 12 additions and 1 deletion.
13 changes: 12 additions & 1 deletion src/main/java/hudson/plugins/perforce/PerforceSCM.java
View file
Expand Up @@ -826,6 +826,8 @@ public boolean checkout(AbstractBuild build, Launcher launcher,
"P4DISABLESYNCONLY", build, this.disableSyncOnly);
disableSyncOnly = overrideWithBooleanParameter(
"P4DISABLESYNC", build, this.disableSyncOnly);
boolean oneChangelistOnly = overrideWithBooleanParameter(
"P4ONECHANGELIST", build, false);

// If we're doing a matrix build, we should always force sync.
if ((Object)build instanceof MatrixBuild || (Object)build instanceof MatrixRun) {
Expand Down Expand Up @@ -962,10 +964,19 @@ public boolean checkout(AbstractBuild build, Launcher launcher,
}
}

// Set newestChange down to the next available changeset if we're building one change at a time
if (oneChangelistOnly && build.getPreviousBuild() != null
&& lastChange > 0 && newestChange > lastChange) {
List<Integer> changeNumbersToBuild = depot.getChanges().getChangeNumbersTo(p4WorkspacePath, lastChange+1);
newestChange = changeNumbersToBuild.get(changeNumbersToBuild.size()-1);
log.println("Remaining changes: " + changeNumbersToBuild);
log.println("Building next changeset in sequence: " + newestChange);
}

if (build instanceof MatrixRun) {
newestChange = getOrSetMatrixChangeSet(build, depot, newestChange, projectPath, log);
}

if (lastChange <= 0) {
lastChange = newestChange - MAX_CHANGESETS_ON_FIRST_BUILD;
if (lastChange < 0) {
Expand Down

5 comments on commit f5c430c

@jkapica
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. p4Label, p4UpstreamProject and p4Counter are explicit in their nature and should take precedence over implicit oneChangelistOnly. This is reflected in the "if/elseif/else" block structure. That's why I have put it there. Current placement is bad, at least without extending the if condition.
  2. I realized now that the oneChangelist is not taking into account "excludes", which should also take precedence.
  3. I'm concerned about the "//..." fallback. Although I'm not convinced it's needed for anything else than the first run.
  4. (Cosmetic) changeNumbersToBuild it not concise. We are going to build only one. I prefer to leave the name workspaceChanges.

I'd have to think more about this, but especially considering (1), this patch doesn't look right.

@rpetti
Copy link
Member Author

@rpetti rpetti commented on f5c430c Nov 21, 2013

Choose a reason for hiding this comment

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

  1. My primary motivation for not putting it in the if/else chain is readability, which the plugin desperately needs right now (especially in this function). You are correct though, and unfortunately that means that it needs to be moved back into that ridiculous looking if/else chain.
  2. Not sure I agree here. The changelist to build (prior to this checkin) was determined using the entire workspace. This should probably stay consistent with that behavior, even if it means changing it. I'd have to think about it some more.
  3. I honestly can't recall the reason for the fallback. I believe there were some issues earlier with changes not being found with certain workspace specs. That's outside the scope of this checkin, though.
  4. Whatever works. I used changeNumbersToBuild because that's exactly what it was advertized as being when printed in the log. We can change it to workspaceChanges if you'd like.

@rpetti
Copy link
Member Author

@rpetti rpetti commented on f5c430c Nov 21, 2013

Choose a reason for hiding this comment

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

  1. Actually, looking at the code again, this makes more sense to me. If oneChangelistOnly is enabled, it will incrementally build towards whatever target has been set, be it a label, a counter, the last build of another project, or simply the latest. I'm more inclined to leave it as it is if you agree.

@rpetti
Copy link
Member Author

@rpetti rpetti commented on f5c430c Nov 21, 2013

Choose a reason for hiding this comment

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

4 has been addressed in a404396.

@jkapica
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Actually, indeed. Although probably not towards last build of another project, but that shouldn't matter, at least it doesn't for me ;>
    I will have to run few tests in the end, but I believe it shall work just fine. Let's leave it there.
  2. I'd say the algorithm shall iterate (from size()-1 to 0) through workspaceChanges variable breaking on first non excluded changelist (isChangelistExcluded), rather than just picking directly next (size()-1). Since workspaceChanges processed this way ends (index 0) with latest, the alternative break condition have to be when reaching the newestChange as calculated earlier (be it a label, counter or latest).
    That should be consistent with polling algorithm in getCurrentDepotRevisionState(). Pooling will jump over all excluded changes, so shall oneChangelist follow this rule. In other words generally speaking, if we have changes to exclude, we shall exclude them also in sequence.
  3. Agree.
  4. Cool.

Please sign in to comment.