Ticket #385 (closed defect: accepted)

Opened 13 months ago

Last modified 7 weeks ago

workarounds for rpm symlink <-> directory issue

Reported by: patches Owned by:
Priority: major Milestone:
Component: Guideline Draft Version:
Keywords: Cc: pmatilai, richardfearn@…, slaanesh
Blocked By: Blocking:

Description

As you might be aware, rpm has problems if you attempt to replace a symlink with a directory or vice versa.

Since this is a fairly common thing to do, and it will only become more so as we unbundle JS libraries from packages and replace them with a symlink for compatibility, it would be nice to have an official guideline for the %pretrans workarounds required to make this work.

So, I've composed a draft here: https://fedoraproject.org/wiki/User:Patches/PackagingDrafts/Symlink_Workarounds

I'm not sure where best to place this, it seems like it would fit nicely in either the File and Directory Ownership section or the %pretrans scriptlet section. I'll leave it up to you all to figure out where you best think it fits.

I copied the snippets from Panu's e-mails on the subject so hopefully they're correct, but I've added him on CC so they can get one final look before going in the official guidelines. :-)

Thanks!

Change History

comment:1 Changed 13 months ago by pmatilai

The scriptlets are okay I guess... if you really really have to do symlink <-> directory conversion and really can't work around it by moving things to different place or such. I'd just like to see some sort of warning to go with it as these %pretrans tricks are not really safe.

Also note there's a bug in rpm at least 4.11.[01] which actually prevents directory symlink -> directory replacement no matter what you do in %pretrans. Technically the bug has been there since rpm 4.7.x but its probably been dormant until 4.11. It should be fixed in rpm >= 4.11.2 though so current Fedora versions will be getting it as an update sooner than later: http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=bcba176fbf0d2cae4d0ee01c0f1b19dde181fc59

As for avoiding these things in the first place... While its obviously not possible to foresee all the cases where the need might arise, when the need *is* foreseeable such as with bundled libraries, it'd be better to use a symlink from the start, as the symlink target can be changed without dirty tricks. Eg if you have a bundled libfoo library inside the packages directory structure, place it to eg libfoo.bundled directory and make libfoo a symlink to that. When the bundling is eventually removed, you just need to drop the directory and change the symlink to point to he corresponding system library directory, no %pretrans tricks required.

comment:2 Changed 13 months ago by pmatilai

Reading through yesterdays FPC meeting log, the guideline clearly needs explanations and not just magic recipes which people see as alien (wtf lua? etc) and are likely to just ignore because it doesn't make any sense to them.

The only thing that makes Lua special with %pretrans is that rpm has an EMBEDDED Lua interpreter which is invoked with -p <lua>. Using external lua, ie -p /usr/bin/lua would fail in %pretrans on initial install just as miserably as the default shell (-p /bin/sh) or anything else. The only thing that can execute in the void that %pretrans executes in initial install is -p <lua> scripts. http://rpm.org/wiki/PackagerDocs/RpmLua has further info on the topic.

Symlink removal from Lua is easy, but it has no support for recursive directory removal. Sure its possible to write code that does it, but its not worth the trouble here: %pretrans scriptlets must use -p <lua> to survive initial system installation, but there are never directory <-> symlink replacements to do on initial installation, only upgrades will ever need that. And on upgrades a shell will be there (or it would be one very, very ill system), so shelling out with os.execute("rm -rf <path") is just fine. The big twist here is that a good ol' /bin/sh script would work in the cases when dir <-> symlink replacement is needed, BUT it would fail in the case where its NOT needed (initial system install). And to test whether replacement is needed, you need some interpreter to be present, and -p <lua> is the only thing that will always be there.

As for 'rm -rf' being risky - sure it is. 'rm -rf' doesn't follow symlinks so its a reasonably "contained" operation from that POV, but you'd better be damn sure the directory is really yours to delete (ie not shared by other packages), that there are no %config files etc. Axing a bundled library or some other truly "private" part of a package itself where no user-created content is expected might be "reasonably safe" but I wouldn't go rm -rf'ing anything at all in eg /etc.

Oh and wrt the inevitable "can't we just use %pre?" remark: no you can't, generally speaking. By the time %pre/%post etc run, rpm has long long since decided what to do about the files and directories in the transaction and changing things underneath can have funky side-effects like your just-installed files getting removed etc. %pretrans runs *before* rpm does its file disposition calculations, and thus will be aware of changes done in %pretrans.

Last edited 13 months ago by pmatilai (previous) (diff)

comment:3 Changed 13 months ago by richardfearn

  • Cc richardfearn@… added

comment:4 Changed 13 months ago by slaanesh

  • Cc slaanesh added

Adding myself in CC as I also faced the mentioned bug:

https://bugzilla.redhat.com/show_bug.cgi?id=975909

Waiting to make a test with rpm-4.11.2-0.rc2.1.fc21 as soon as I find some (little damn) time.

comment:5 Changed 12 months ago by toshio

FPC discussed this a few weeks ago: http://meetbot.fedoraproject.org/teams/fpc/fpc.2014-01-23-17.02.log.html

We were a little concerned about the possibility that files that the sysadmin wanted to protect would get deleted (for instance, config files) or that doing this would break other packages (for instance, plugins). Some things I'd like to see added to/changed in the draft (rephrased as appropriate):

From Panu's comment:1 When you can foresee that you might need to switch symlink and directory in the future "it'd be better to use a symlink from the start, as the symlink target can be changed without dirty tricks"

From Panu's comment:2 "%pretrans scriptlets must use -p <lua> to survive initial system installation, but there are never directory <-> symlink replacements to do on initial installation, only upgrades will ever need that. And on upgrades a shell will be there. This means that we need to use -p <lua> in order to test if the replacement needs to be made but we can use os.execute() to actually perform the replacement."

From Panu's comment:2 "but you'd better be damn sure the directory is really yours to delete (ie not shared by other packages), that there are no %config files etc". Mention something about when not to use these tricks: not where %config files could be (/etc is an indication you shouldn't use this) because this would remove files that the user might have edited, not where statefiles could be (/var/ is similarly an indication that you shouldn't be doing this) as this would remove files that might have been created by the program, not if other packages might be installing files (example: plugins) in there as this will leave the other packages broken.

Additionally, removing in the directory=>symlink case still seems dangerous. I'd prefer to rename a directory rather than remove it. Renaming does mean that someone would need to come by and clean it up later but: 1) the same could happen with the directory pointed to by a symlink 2) it's better than losing something important to the user. You could also try moving the contents of the renamed directory into the directory pointed to by the symlink and then remove the directory but you'd have to be careful of corner cases (doesn't work if the symlink is no longer pointing to a directory, if the symlink points to a location that isn't on the same disk/partition then there's a chance that you'd run out of space on the new partition, etc). You'd also have to be careful of the case where the directory you're renaming to already exists. You could possibly rename the directory using tempfile. Or possibly mimic rpm's handling of %config files (name the backup $filename.rpmsave and if that filname already exists, remove the old $filename.rpmsave before renaming).

@panu do you see reasons that some sort of renaming strategy would be more dangerous than removing the directory?

comment:6 Changed 12 months ago by pmatilai

Well, certainly renaming is the safer option, and also makes the %pretrans snippet less black magic and more like the symlink -> directory version, something like:

%pretrans -p <lua>
st = posix.stat("<path to dir>")
if st and st.type == "directory" then
   os.rename("<path to dir>", "<path to dir>.<suffix>")
end

If a predictable suffix is used, the renamed directory could included as %ghost in the new package so the directory isn't entirely orphaned, and in best case (if the directory is empty) will even get removed on package erasure. The %ghost approach could be taken further too - if the entire directory with its contents (assuming its a package private directory) is packaged as %ghost then the whole thing will be removed eventually.

comment:7 Changed 12 months ago by patches

I've now modified the draft incorporating everyone's suggestions. See the new version here or check the log for specific changes.

While writing the suggestion for renaming in the directory=>symlink case, I decided to go with the '.rpmsave' suffix since it's something many end users are familiar with. Using a predictable sufffix also makes the %ghost thing Panu mentions possible, and indeed I added some text recommending packagers do that too.

If we are to adopt the renaming strategy, I think it's important to inform end users that we are now doing this via the F21 release notes, so nobody will be shocked to see that they now occasionally get .rpmsave directories on their systems in addition to that happening with config files. After this is approved by FPC I'll file a bug with the docs team to make sure that gets done.

comment:8 follow-up: ↓ 9 Changed 12 months ago by pmatilai

I'm not sure using .rpmsave for this is a good idea as the case is so different - .rpmsave indicates a config file which might need merging, but here its a directory containing stuff, replaced by a symlink to another directory ... who knows what sort of creative ideas users might get ;) Including making a bigger mess by trying to somehow merge stuff, filing bugs about rpm creating .rpmsave of non-config items etc...

A minor correction wrt the actual issue - the limitations are, to be precise:

  • a directory cannot be replaced by anything else (ie this is not limited to symlinks)
  • a symlink to *a directory* cannot be replaced by a directory

In the scriptlet for replacing a directory symlink with a directory, manually creating the directory is unnecessary (but mostly harmless) as rpm will create it when the time comes, just like any other directory.

comment:9 in reply to: ↑ 8 Changed 12 months ago by patches

Replying to pmatilai:

I'm not sure using .rpmsave for this is a good idea as the case is so different - .rpmsave indicates a config file which might need merging, but here its a directory containing stuff, replaced by a symlink to another directory ... who knows what sort of creative ideas users might get ;) Including making a bigger mess by trying to somehow merge stuff, filing bugs about rpm creating .rpmsave of non-config items etc...

Hmm, good point, but I'm not sure what to use instead. Toshio did suggest using tmpfile for a random string but then we can't use your %ghost suggesstion. :-(

I think I'll just leave it alone and let FPC make the final call at their meeting; choosing bikeshed colors is after all their raison d'etre. :-P

A minor correction wrt the actual issue - the limitations are, to be precise:

  • a directory cannot be replaced by anything else (ie this is not limited to symlinks)
  • a symlink to *a directory* cannot be replaced by a directory

I changed the draft to reflect this.

In the scriptlet for replacing a directory symlink with a directory, manually creating the directory is unnecessary (but mostly harmless) as rpm will create it when the time comes, just like any other directory.

Also fixed. :-)

comment:10 Changed 12 months ago by toshio

Looks like we'll have to handle the "<dir>.rpmsave already exists" case somehow as os.rename('dir1', 'dir2') fails if dir2 already exists.

/me looking into it

comment:12 Changed 12 months ago by patches

Maybe we should add a variable for the path at the top and use it in the rest? Requiring packagers to change the directory in seven places is just asking for us to accidentally miss one. ;-)

Also, does %ghost accept globs? Should we recommend using something like "%ghost /path/to/dir.rpmsave*" to catch the potential other .rpmsave directories?

comment:13 Changed 12 months ago by pmatilai

Seems like my mumbling against .rpmsave was not clear enough. Having slept over this:

Please, please do not use .rpmsave as the rename-suffix. Please.

  • It has a very specific meaning in rpm: .rpmsave is a backup of a *user-modified* %config file which you do NOT want to blindly throw away, the renamed directory here is nothing like that at all.
  • There are tools and adhoc scripts that look for .rpmnew and .rpmsave files, some even try to merge the content etc, which you do NOT want to happen here.
  • Packaging .rpmsave files, even as %ghost, looks mighty peculiar.
  • .rpmsave|.rpmorig|.rpmnew are all indications of an action taken by *rpm* actions, on %config files, whereas these are *package* actions on their directories. I dont want to see "rpm creates .rpmsave backups for non-config files" bugs filed for package scriptlets.

I'd recommend using something totally different and unique for this case, something which hints at what it is and doesn't look totally weird and wrong when packaged. Say, a derivate of .bundled or sumthin.

Also the description of the limitation is still incorrect wrt symlink replacement: the problem case is really as specific as replacing a symlink to a directory, with a directory. Even replacing a symlink to a directory with a symlink to another directory is not a problem. Please be specific in the description, we dont want people get crazy with %pretrans when just because symlinks are involved :)

%ghost accepts globs and whole directories, but note that such globs apply to build-time, not package removal. So you could do something like this in the spec:

%install
%make_install # or whatever...

mv %{buildroot}/path/to/dir %{buildroot}/path/to/dir.wasbundled

%files
...
%ghost /path/to/dir.wasbundled/

That way the entire directory with its contents would be ghosted and removed on package removal, IF the directory contents are the same between the old and new package version. But certainly you can manually construct the paths to be ghosted if necessary. Obviously you should replace the .wasbundled by whatever suffix ends up in the guideline.

comment:14 Changed 12 months ago by toshio

suffix: I agree that being a directory and %ghost'd is very different than config files. But to an enduser this being caused by the package's scripts vs internal rpm logic isn't very obvious. Maybe something like .rpmmoved which isn't going to be used by current scripts but could be found with file globs?

I'll take a stab at the description.

comment:15 follow-up: ↓ 16 Changed 12 months ago by patches

How's this?

Part of it states the obvious but it ought to eliminate any possibility of confusion. (Except of course if I'm still confused. ;-)

comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 12 months ago by pmatilai

Replying to toshio:

suffix: I agree that being a directory and %ghost'd is very different than config files. But to an enduser this being caused by the package's scripts vs internal rpm logic isn't very obvious. Maybe something like .rpmmoved which isn't going to be used by current scripts but could be found with file globs?

No objections to .rpmmoved, it matches the two important criteria of being easily searchable and distinct from the %config file cases.

Replying to patches:

How's this?

Part of it states the obvious but it ought to eliminate any possibility of confusion.

That's perhaps a bit over the top, I dont think this guide is directed at "do not use the microware to dry pets"-type audience ;) "Only use this for..." and the accurate descriptions of the limits should suffice. Too much "IMPORTANT IMPORTANT" text just causes inflation / nobody reading it.

comment:17 in reply to: ↑ 16 Changed 12 months ago by patches

Replying to pmatilai:

That's perhaps a bit over the top

More like way over the top. ;-)

Better now?

comment:18 Changed 12 months ago by james

Using .rpmmoved seems fine to me, using the number approach to make sure we always move everything ... also fine.

comment:19 Changed 12 months ago by toshio

#info workaround for rpm symlink <-> directory issues approved (+1:7, 0:0, -1:0)

comment:20 Changed 12 months ago by toshio

  • Status changed from new to writeup

Written into guidelines here: https://fedoraproject.org/wiki/Packaging:Directory_Replacement with a link to them in the Main Guidelines page: https://fedoraproject.org/wiki/Packaging:Guidelines#Replacing_a_symlink_to_a_directory_or_a_directory_to_any_type_file

Announcement text:

"""

A longtime limitation of RPM is that it cannot replace a directory with any kind of file nor can it replace a symlink to a directory with a directory. Workarounds with varying limitations have been used in different packages when the need has arisen. We now have guidelines that codify one of these workarounds that we hope has as few problems as possible and also documents the known limitations of its approach. The guideline also encourages has advice on designing your package to not run into this problem down the road in cases where the problem can be anticipated in advance.

Please see: https://fedoraproject.org/wiki/Packaging:Directory_Replacement for information in case you are in the unfortunate position of having to deal with this case.

"""

comment:21 Changed 3 months ago by tibbs

  • Status changed from writeup to announce

comment:22 Changed 7 weeks ago by tibbs

  • Resolution set to accepted
  • Status changed from announce to closed
Note: See TracTickets for help on using tickets.