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.

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

Clean workspace before building incrementals #141

Merged

Conversation

MarkEWaite
Copy link
Contributor

@MarkEWaite MarkEWaite commented Apr 11, 2020

Clean workspace before building incrementals

The incrementals build process will abort if git status -s is not empty. Since the maven step will invoke mvn clean, a git clean is safe. In case command line git is not installed, the script does its very best to not fail with an error and to report that it intentionally did not fail with an error.

Also includes a spelling fix.

This change is needed until ci.jenkins.io agents become 100% ephemeral. I've been hit repeatedly by tainted agent workspaces with the git plugin. This will reduce the failure rate of git plugin builds and git client plugin builds on ci.jenkins.io.

See my git plugin pull request for an example of this pull request being used in a repository.

The incrementals build process will abort if `git status -s` is not empty.
Since the maven step will invoke `mvn clean`, a `git clean` is safe.
@MarkEWaite MarkEWaite requested a review from a team April 11, 2020 16:52
bitwiseman
bitwiseman previously approved these changes Apr 11, 2020
@bitwiseman
Copy link
Contributor

@jenkins-infra/ci-maintainers Can we merge this?

jglick
jglick previously approved these changes May 11, 2020
@@ -66,6 +66,17 @@ def call(Map params = [:]) {
isMaven = fileExists('pom.xml')
incrementals = fileExists('.mvn/extensions.xml') &&
readFile('.mvn/extensions.xml').contains('git-changelist-maven-extension')
if (incrementals) { // Incrementals needs 'git status -s' to be empty at start of job
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not just do this unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We certainly could, but I wanted to keep the impact of the change to only those specific cases where I know it is an issue. I'm willing to change the pull request to do it in all cases if that will help with its approval.

if (isUnix()) {
sh(script: 'git clean -xffd || true',
label:'Clean for incrementals',
returnStatus: true) // Ignore failure if CLI git is not available
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the || true not sufice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, but I was practicing an "abundance of caution" in hopes that a peripheral cleanup operation would have even less chance of inadvertently breaking a build.

Copy link
Contributor

Choose a reason for hiding this comment

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

So then returnStatus: true should suffice (both both platforms)?

Copy link
Contributor Author

@MarkEWaite MarkEWaite May 12, 2020

Choose a reason for hiding this comment

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

Yes, should. Same "abundance of caution" logic. If ignoring the return value inside the script somehow did not work, also take the active measure of ignoring the return value from the step. Both sh and bat step documentation say:

Normally, a script which exits with a nonzero status code will cause the step to fail with an exception. If this option is checked, the return value of the step will instead be the status code.

Copy link
Contributor Author

@MarkEWaite MarkEWaite May 16, 2020

Choose a reason for hiding this comment

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

I've changed to hide the output from git clean and ignore the return code in c05194b . Let me know if other changes are needed (after the fix to change the Windows null device to the correct name nul instead of the incorrect name null)

No intention to do anything with the output if there is a failure.  If
the `git clean` command fails, then the `git status -s` command inside
the maven incrementals build will likely also fail.

Avoids distracting typical users reading the log file and wondering
about output messages from `git clean`.
@MarkEWaite MarkEWaite dismissed stale reviews from jglick and bitwiseman via c05194b May 16, 2020 14:44
jglick
jglick previously approved these changes May 19, 2020
Copy link
Contributor

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Fine if these commands have been tested and seem to work. Currently there is no mechanism for testing buildPlugin AFAIK.

@MarkEWaite
Copy link
Contributor Author

Fine if these commands have been tested and seem to work. Currently there is no mechanism for testing buildPlugin AFAIK.

I tested them by running them on my home CI server. The CI tests at home (SCM API, Branch API, Git plugin, Git client plugin, Platform labeler plugin) detected the mistake that I had made in c05194b of using the wrong name for the nul device on Windows. That is corrected in 09edbc1. It was quite awkward to have a file named null on Windows causing 100% failure of all jobs that used incrementals.

@bitwiseman
Copy link
Contributor

@jenkins-infra/ci-maintainers
Can we merge this now? My CI jobs continue to fail due to files left in the workspace.

@MarkEWaite
Copy link
Contributor Author

@jenkins-infra/ci-maintainers
Can we merge this now? My CI jobs continue to fail due to files left in the workspace.

I had to resolve a conflict. I'm not sure that I chose the correct direction for the resolution of the conflict. We'll know when the tests run.

@jglick
Copy link
Contributor

jglick commented May 28, 2020

I'm not sure that I chose the correct direction for the resolution of the conflict.

Pro tip (cannot fathom why this is not the default): add to ~/.gitconfig

[merge]
	conflictstyle = diff3

MarkEWaite added a commit to MarkEWaite/git-plugin that referenced this pull request Jun 4, 2020
Once jenkins-infra/pipeline-library#141 is merged,
the calls to `clean` can be removed.
@MarkEWaite
Copy link
Contributor Author

MarkEWaite commented Jun 4, 2020

I continue to encounter this problem in plugin builds that use incrementals. Is there anything else blocking it from being included in the pipeline shared library?

@jenkins-infra/ci-maintainers is there anything that I need to do to have this merged?

@timja
Copy link
Member

timja commented Jun 7, 2020

@MarkEWaite imo you should request to join that team, you’re doing work on ci.jenkins.io regularly

@olblak olblak merged commit 1dcc297 into jenkins-infra:master Jun 8, 2020
@MarkEWaite MarkEWaite deleted the clean-workspace-if-incrementals branch June 8, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants