FS#53770 - emptydirs runs too early

Attached to Project: Pacman
Opened by Luke Shumaker (lukeshu) - Monday, 24 April 2017, 01:26 GMT
Last edited by Allan McRae (Allan) - Tuesday, 29 December 2020, 13:40 GMT
Task Type Bug Report
Category makepkg
Status Assigned
Assigned To Allan McRae (Allan)
Architecture All
Severity Low
Priority Normal
Reported Version 5.0.1
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 11
Private No

Details

Summary and Info:

The tidy_remove hooks are run in sorted-filename order. This means that, several other remove hooks are run after tidy_emptydirs (namely, tidy_libtools, tidy_purge, and tidy_staticlibs). Each of these may remove files, making a directory empty; and tidy_emptydirs would like to be able to remove them.

Steps to Reproduce:

Make the 'perl-mldbm' package from AUR. Note that despite the '!emptydirs' being included in options, that there are several empty directories under '/usr/lib'. This is because with the default PURGE_TARGETS value, tidy_purge wipes out every file that perl-mldbm places under '/usr/lib' ('.packlist', and '*.pod')

The obvious solution would be to put "NN-" prefixes on the tidy hook filenames, and give emptydirs.sh a high NN.
This task depends upon

Comment by Eli Schwartz (eschwartz) - Wednesday, 11 October 2017, 19:20 GMT
But the obvious solution still depends on the order bash happens to sort globbed expansions, which in turn depends on LC_COLLATE. But we don't document any order at all, which means we shouldn't be relying on one.

An alternative solution would be to have e.g. tidy_remove_late() for hooks which depend on the actions of other hooks, and fit tidy_emptydirs() there instead.
Comment by Moritz Bunkus (mbunkus) - Monday, 16 October 2017, 07:29 GMT
LC_COLLATE? Seriously? Just prefix all scripts with a unique two-digit number. That's what everyone else does, because it Just Works.
Comment by Anton Leontiev (bunder) - Wednesday, 08 August 2018, 18:44 GMT
Really annoying issue. Two-digit prefix will work for any LC_COLLATE, because arabic numerals are sorted identically for any LC_COLLATE, IHMO. I'm not a big fan of digits, but tidy_remove_late() seems to be too overengineered.
Comment by Luke Shumaker (lukeshu) - Thursday, 09 August 2018, 18:11 GMT
> But we don't document any order at all, which means we shouldn't be relying on one.

I know Allan has been talking about "makepkg drop-ins", but I don't think anything about /usr/share/makepkg/tidy/ is documented. I don't see as much of a problem changing an undocumented interface.

I also agree with Anton. I'd be very surprised if there are any locales that sort Arabic numerals differently (I'm figuring out how to parse /usr/share/i18n/locales/ to verify that). And even if there are, just save/restore LC_COLLATE and LC_ALL around the "$LIBRARY/tidy/"*.sh glob.
Comment by Eli Schwartz (eschwartz) - Thursday, 09 August 2018, 18:21 GMT
FS#58769 :D

The problem with just doing undocumented fixes for this is that future refactoring might accidentally break it because it's not obvious that it matters. I'm not worried about *breaking* an interface (that is already broken).
Comment by Luke Shumaker (lukeshu) - Thursday, 09 August 2018, 18:31 GMT
I didn't remember the locale definition file-format as being part of POSIX, but I was mistaken (I'm blaming it on only skimming volume 1, most of the juicy stuff is later). POSIX has truly user-defined locales; it isn't safe to rely on sane ordering of the Arabic numerals (so just save/restore LC_COLLATE and LC_ALL).

If everything else is "10-" and emptydirs is "99-", that's a good hint that "maybe this needs to run later". Even more so if you add a comment saying "this must run late because other tidy_remove hooks might remove files making a directory empty".
Comment by AMM (amish) - Wednesday, 12 December 2018, 02:11 GMT
Easy fix would be to change /usr/share/makepkg/tidy/emptydirs.sh:

- tidy_remove+=('tidy_emptydirs')
+ tidy_modify+=('tidy_emptydirs')

Since modify rules are run after remove rules - emptydirs will run after all remove rules are run.

We can add a comment in code stating that emptydirs is added in modify rules to make sure it runs after all remove rules.

Loading...