Skip to content

publish-crates: avoid cleaning up the tmp folder on CI#32

Merged
joao-paulo-parity merged 1 commit intoparitytech:masterfrom
joao-paulo-parity:fix
Feb 9, 2023
Merged

publish-crates: avoid cleaning up the tmp folder on CI#32
joao-paulo-parity merged 1 commit intoparitytech:masterfrom
joao-paulo-parity:fix

Conversation

@joao-paulo-parity
Copy link
Contributor

@joao-paulo-parity joao-paulo-parity commented Feb 9, 2023

The rm: cannot remove XXX: Directory not empty message is misleading. The error occurs not because the directory has something in it, but because some process is attached to a file within the directory, which makes it so it cannot be removed. See for instance https://stackoverflow.com/questions/64852408/cannot-remove-git-directory-not-empty.

On CI it's not necessary to clean up the temporary folder because it'll be discarded automatically once the job runner exits. The error can still happen for non-CI environments, but I'll punt on handling that for now because the script is not supposed to be run outside of CI (there's a clear warning about that in the comment at the start of the script).

closes #31

@joao-paulo-parity joao-paulo-parity requested a review from a team as a code owner February 9, 2023 10:13
@joao-paulo-parity joao-paulo-parity merged commit ad3d627 into paritytech:master Feb 9, 2023
@joao-paulo-parity joao-paulo-parity deleted the fix branch February 9, 2023 10:14
@chevdor
Copy link
Contributor

chevdor commented Feb 9, 2023

Why not running pkill first and then rm -rf ... || true ?
it would have been more concise and this way we attempt a cleanup and give up if there is a non critical issue.

@joao-paulo-parity
Copy link
Contributor Author

joao-paulo-parity commented Feb 9, 2023

Why not running pkill first and then rm -rf ... || true ?

fuser is better than kill for this, as mentioned in https://stackoverflow.com/a/64860231/16833094. Or, more crudely, a rm loop up to N retries with sleeps in-between (until the process lets go of the file it's attached to) could also work.

it would have been more concise and this way we attempt a cleanup and give up if there is a non critical issue.

I thought of implementing a workaround, but I stopped myself precisely because it would not be more concise; on the contrary, more code would be added for handling the "problem" - which is actually not a problem because the directory doesn't need to be cleaned up on CI. It seemed sensible to sidestep the procedure rather than add more code for handling what's effectively a non-issue for CI environments.

Indeed, the situation is not solved for non-CI environments. To reiterate:

I'll punt on handling that for now because the script is not supposed to be run locally (there's a clear warning about that in the comment at the start of the script)

Regardless, I've created a ticket for enhancing this part of the script such that it also behaves well on non-CI environments (although I doubt it'll ever be used that way).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crates CI job fails

2 participants