cosalib/build: Use workdir tmp/ as tempdir #1097
Conversation
We changed this behaviour during the refactor. There's quite a bit of history here on why do this. But a major one at least is that we want to be able to just `rename(2)` the final build artifacts into place. This saves a bunch of time and I/O. I noticed this due to the fact that we were losing sparsity from the output of `qemu-img convert` because `shutil.move` doesn't do the equivalent of `cp --sparse=auto`. This patch fixes that, though I think we should also be able to change that call to a simple `os.rename()` in a follow-up to make it explicit. Related: coreos/fedora-coreos-tracker#361
|
Oh yes, thanks for fixing this! I thought I saw this bug while reading the code but forgot about it. Besides just being faster, each GB we avoid writing speeds up development time and increases SSD life. |
| @@ -96,7 +96,12 @@ def __init__(self, *args, **kwargs): | |||
|
|
|||
| self._found_files = {} | |||
| self._workdir = kwargs.pop("workdir", os.getcwd()) | |||
| self._tmpdir = tempfile.mkdtemp(prefix="build_tmpd") | |||
| tmpdir = os.path.join(self._workdir, "tmp") | |||
| os.environ['TMPDIR'] = tmpdir | |||
There was a problem hiding this comment.
This isn't new, but... rust-lang/rust#24741
AFAIK cosa isn't multi-threaded today, but it's just a time bomb to be mutating the environment. We can fix as a follow-up.
There was a problem hiding this comment.
Yeah, we should probably just set it in the entrypoint instead?
There was a problem hiding this comment.
Hmm, I think it's trickier than that since the entrypoint has no notion of workdir. So yeah, let's do that as a follow-up!
|
https://github.com/coreos/coreos-assembler/blob/master/src/cosalib/build.py#L130-L137 Probably want to whack that too. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: darkmuggle, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
https://github.com/darkmuggle/coreos-assembler/commit/4415410fdc8a2b8178ae8ced09c67d804b1a8aa4 Given that this PR is approved, I'd vote we merge this one ASAP since CI is affected. |
Hmm, we still want to clean up the tmpdir though, right?
I think it's mostly equivalent, I just deduped it with the path we were already building when setting OK, going to merge this one manually for now until we sort out the Prow issues! |
We changed this behaviour during the refactor. There's quite a bit of
history here on why do this. But a major one at least is that we want to
be able to just
rename(2)the final build artifacts into place. Thissaves a bunch of time and I/O.
I noticed this due to the fact that we were losing sparsity from the
output of
qemu-img convertbecauseshutil.movedoesn't do theequivalent of
cp --sparse=auto. This patch fixes that, though I thinkwe should also be able to change that call to a simple
os.rename()ina follow-up to make it explicit.
Related: coreos/fedora-coreos-tracker#361