The try/except commit seems OK, but I'm not sure about the second one ...
I'd prefer to work with only one tmp directory, and re-use it in
Provider (currently there are two temp directories, handled on two separate
places, which is easy to break, and worse for control - e.g. we need to make
sure that there's enough space).

That said, I'm OK with this PR, +1. But using one TMP dir would be better,
though we/I can do the rest in separate PR.

@praiskup I'd prefer to work with only one tmp directory, and re-use it in Provider

That was actually my original approach. it is a one-liner, so if you prefer this one, I will change it. Basically you need to do

self.workdir = tempfile.mkdtemp(dir=outdir)

and then you can remove both cleanup and __del__ methods.

can you add prefix= here?

self.workdir = tempfile.mkdtemp(dir=outdir)

Do we need additional level of directory tree at all?

can you add prefix= here?

I've added prefixes for all copr-rpmbuild temporary directories

Do we need additional level of directory tree at all?

I just tried that with self.workdir = outdir and it works fine for producing RPMs, but it breaks

copr-rpmbuild --srpm --task-url ...

Of course, it could be fixed, but we would have to filter what files we want to copy to resultdir in main.py:produce_srpm(...). IDK whether it is worh it.

--task-url

I'm not sure this is ever used.

Of course, it could be fixed, but we would have to filter what files we want to copy to resultdir in main.py:produce_srpm(...). IDK whether it is worh it.

Ok, if we created subdir with predictable name, say join(outdir, 'workdir')? If we
could avoid the need to explicitly remove another subdirectory, it at least sounds
temptingly!

--task-url

I'm not sure this is ever used.

It doesn't break the --task-url, there is no problem with that. The --srpm stops working.

Aha, but can we avoid using mkdtemp() and add just mkdir() instead, and drop the del/cleanup hell?

PTAL

it is not obvious shutil is needed

Would you mind adding an unittest for (a) random traceback, (b) build success and (c) build failure that the directory is really removed?

Otherwise lgtm! Thank you.

Will do, thank you for the review.

One more note, TOCTOU, it's always better to create the directory without asking.

Looks good otherwise.

Do you really want to have this on 6 commits? (no objection, I just wouldn't personally)?
Do you plan to add the new tests (a, b, c ^^)?

I'm asking because we are in hurry now before release, and I'm going to merge this
today.

Then tests break because multiple of them want to use the same resultdir and you get an exception that the directory already exists. I thought it would be better to handle like this, rather than figure out some cleaning in tests.

I will definitely add the tests, but if you don't mind merge this one without them. I am not sure when I get to a computer and I don't want to block your release.

I meant the classic idiom: try: mkdir(): except exists: pass

Ah, give me a second. I can do that.

Don't you know why this collides? :-) This sounds like another pagure mystery.

No, there really is collision in rpmbuild/run_tests.sh.

Commit ad5422e fixes this pull-request

Hehe, because you apparently did the exact same change (4a8f0d4) as I do, but few days earlier :-). I am removing it.

I merged manually :-) this PR actually broke the testsuite for epel-7/epel-6. I can fix it in separate commit this evening, and wrap new release if you don't have time. But basically, it should be ready for beaker tests.