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

Ensure %clean always succeeds #2519

Closed
praiskup opened this issue May 22, 2023 · 4 comments · Fixed by #3006
Closed

Ensure %clean always succeeds #2519

praiskup opened this issue May 22, 2023 · 4 comments · Fixed by #3006
Labels
bug handsfree Packaging automation and convenience

Comments

@praiskup
Copy link
Member

Could the default %clean script automatically change the mode of directories
before doing rm -rf? To avoid build failures caused by unsuccessful rm calls:

$ mkdir nonremovable
$ touch nonremovable/protect-parent
$ chmod a-r nonremovable
$ rm -rf nonremovable/
rm: cannot remove 'nonremovable/': Permission denied

Note this problem can cause problems Mock/Copr: See also rpm-software-management/mock#1081

@pmatilai
Copy link
Member

pmatilai commented Jun 1, 2023

Yeah, rpm itself suffers from this too. Both from the POV that rpmbuild may leave cruft behind (which does look like a bug when you look at it that way), but also when building rpm itself the test-suite can be non-removable afterwards.

@pmatilai pmatilai added the bug label Jun 1, 2023
@pmatilai pmatilai added the handsfree Packaging automation and convenience label Sep 14, 2023
@dmnks
Copy link
Contributor

dmnks commented Oct 4, 2023

FTR, CMake does something similar on make clean as I discovered the other day. As for the test-suite artifacts, we already do exactly that. It just seems natural to have %clean follow suit...

@dmnks dmnks assigned dmnks and unassigned dmnks Oct 4, 2023
@pmatilai
Copy link
Member

pmatilai commented Mar 20, 2024

Another case where we'll run into this is if/when we add support for vpath builds in read-only tree (#2985).

The default %clean is a macro now, making this kind of thing easy.

@pmatilai pmatilai changed the title Could the default %clean section remove non-readable+non-empty directories? Ensure %clean always succeeds Mar 20, 2024
@pmatilai
Copy link
Member

pmatilai commented Mar 21, 2024

Actually the whole %clean thing seems redundant since 9d35c8d because everything is under that one directory and we need to be able to nuke it, no matter what.

Fixing this would be a one-liner to build.c:

     if (what == RPMBUILD_RMBUILD) {
+       fprintf(fp, "chmod -R u+w '%s'\n", spec->buildDir);
        fprintf(fp, "rm -rf '%s'\n", spec->buildDir);

...but because %clean is still trying to do its own thing of removing the buildroot, it fails before it gets to %rmbuild, which would handle it. Doh. Of course we could just add a similar call there but that is silly too. I'll need a fresher head to think if there's still a case for that separate %clean or should we just flush it...

pmatilai added a commit to pmatilai/rpm that referenced this issue Mar 28, 2024
Packages may contain files without write permissions, read-only sources
and whatnot. Run %_fixperms on the whole builddir before trying to
remove it to ensure we don't fail doing our household duties.

It's worth noting that prior to b5f6c64
the build could similarly fail while running it's default %clean.
If we hadn't removeed the default %clean, we'd have to do this
permission fixup first redundantly for the build root and then for the
rest of the %builddir.

Also worth noting is that the added test doesn't actually reproduce the
issue in the test-suite because it runs as root and root is not bothered
by such petty issues as missing read/write permissions to remove. But
at least we have a reproducer for the case once rpm-software-management#3005 is done.

Fixes: rpm-software-management#2519
pmatilai added a commit to pmatilai/rpm that referenced this issue Mar 28, 2024
Packages may contain files without write permissions, read-only sources
and whatnot. Run %_fixperms on the whole builddir before trying to
remove it to ensure we don't fail doing our household duties, both
on post-cleanup but also before trying to remove an old run, as
eg 'rpmbuild -bi' will not run cleanup.

It's worth noting that prior to b5f6c64
the build could similarly fail while running it's default %clean.
If we hadn't removeed the default %clean, we'd have to do this
permission fixup first redundantly for the build root and then for the
rest of the %builddir.

Also worth noting is that the added test doesn't actually reproduce the
issue in the test-suite because it runs as root and root is not bothered
by such petty issues as missing read/write permissions to remove. But
at least we have a reproducer for the case once rpm-software-management#3005 is done.

Fixes: rpm-software-management#2519
pmatilai added a commit to pmatilai/rpm that referenced this issue Apr 4, 2024
Packages may contain files without write permissions, read-only sources
and whatnot. Run %_fixperms on the whole builddir before trying to
remove it to ensure we don't fail doing our household duties, both
on post-cleanup but also before trying to remove an old run, as
eg 'rpmbuild -bi' will not run cleanup.

It's worth noting that prior to b5f6c64
the build could similarly fail while running it's default %clean.
If we hadn't removeed the default %clean, we'd have to do this
permission fixup first redundantly for the build root and then for the
rest of the %builddir.

Also worth noting is that the added test doesn't actually reproduce the
issue in the test-suite because it runs as root and root is not bothered
by such petty issues as missing read/write permissions to remove. But
at least we have a reproducer for the case once rpm-software-management#3005 is done.

Fixes: rpm-software-management#2519
pmatilai added a commit that referenced this issue Apr 4, 2024
Packages may contain files without write permissions, read-only sources
and whatnot. Run %_fixperms on the whole builddir before trying to
remove it to ensure we don't fail doing our household duties, both
on post-cleanup but also before trying to remove an old run, as
eg 'rpmbuild -bi' will not run cleanup.

It's worth noting that prior to b5f6c64
the build could similarly fail while running it's default %clean.
If we hadn't removeed the default %clean, we'd have to do this
permission fixup first redundantly for the build root and then for the
rest of the %builddir.

Also worth noting is that the added test doesn't actually reproduce the
issue in the test-suite because it runs as root and root is not bothered
by such petty issues as missing read/write permissions to remove. But
at least we have a reproducer for the case once #3005 is done.

Fixes: #2519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug handsfree Packaging automation and convenience
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants