Windows docker build ensure cleanWorkspace cleans build tmp workspace

[andrew-m-leonard]

For Windows which builds within a tmp workspace, ensure when CLEAN_WORKSPACE is specified it is honoured.

ci-jenkins-pipelines/pipelines/build/common/openjdk_build_pipeline.groovy

Line 2190 in 207bcc8

if (cleanWorkspace) {

Also, changed pr-tester default setting to cleanWorkspace prior to tester builds, which should now ensure windows host cleans workspace before container build

Some test builds:

• https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-windows-x64-temurin/241/
• https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-windows-aarch64-temurin/34/

[github-actions]

Thank you for creating a pull request!

Please check out the information below if you have not made a pull request here before (or if you need a reminder how things work).

Code Quality and Contributing Guidelines

If you have not done so already, please familiarise yourself with our Contributing Guidelines and Code Of Conduct, even if you have contributed before.

Tests

Github actions will run a set of jobs against your PR that will lint and unit test your changes. Keep an eye out for the results from these on the latest commit you submitted. For more information, please see our testing documentation.

In order to run the advanced pipeline tests (executing a set of mock pipelines), it requires an admin to post run tests on this PR.
If you are not an admin, please ask for one's attention in #infrastructure on Slack or ping one here.
To run full set of tests, use "run tests"; a subset of tests on specific jdk version, use "run tests quick 11,21"

[andrew-m-leonard]

run tests

[sxa]

LGTM. This looks to do the correct thing when the workspace directory is owned by the user in the container's host system due to a problematic earlier run. Noting that for this to be effective CLEAN_WORKSPACE needs to be set to true so perhaps we should also consider changing that to be the default in the PR tester.

[eclipse-temurin-bot]

 PR TESTER RESULT 

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

[sxa]

If I'm reading updates to this PR correctly it's masking some errors and just printing a warning now instead, so potentially continuing even if things aren't being cleaned up, therefore there seems to be a risk of running the builds with out of date things in the workspace.
I feel we should be aborting if such a condition occurs generally - is there a reason to mask the cleanup failure?

Also bear in mind that in the block around rm -rf that is a command that cannot return a failure error code, so I don't believe the catch block can be triggered.

[andrew-m-leonard]

If I'm reading updates to this PR correctly it's masking some errors and just printing a warning now instead, so potentially continuing even if things aren't being cleaned up, therefore there seems to be a risk of running the builds with out of date things in the workspace. I feel we should be aborting if such a condition occurs generally - is there a reason to mask the cleanup failure?

Also bear in mind that in the block around rm -rf that is a command that cannot return a failure error code, so I don't believe the catch block can be triggered.

@sxa Note this is "post-build" cleanup if cleanWorkspaceAfter=true, which I believe will always fail for docker as the ci-jenkins-pipelines|temurin-build repo checkout is done by host, also note the hidden file cleanup was already within a catch clause

Note the outer catch can be for a Jenkins context.timeout

[andrew-m-leonard]

run tests

[sxa]

Also bear in mind that in the block around rm -rf that is a command that cannot return a failure error code

Hmm apparently it does specifically in the Permission denied case. I retract that comment.

[sxa]

this is "post-build" cleanup if cleanWorkspaceAfter=true, which I believe will always fail for docker

Fair point. I'm now not so convinced it should have the ERROR: prefix now, but I'll leave it to you :-)

[andrew-m-leonard]

I'm now not so convinced it should have the ERROR: prefix now, but I'll leave it to you :-)

yeah good point, i'll change to WARNING

[adamfarley]

LGTM

[andrew-m-leonard]

run tests

[eclipse-temurin-bot]

 PR TESTER RESULT 

✅ All pipelines passed! ✅