Move ~/.rpmmacros and ~/.rpmrc to ~/.config/rpm/ #3750
Conversation
|
I wonder if we want to do the move if XDG points to a non standard place. |
|
It shouldn't matter where the location is, as long as RPM reads it properly. |
We actually do that in the surrounding code already. It's just not part of this particular patch. My question is more about what if it is set temporarily and we move the files at some weird place. But then we still have them symlinked from the home so that should be ok and still work if the |
|
Yeah. I think worrying about that problem is out of scope for us. The expectation with that variable is that it is environment-persistent (that is, if it is set, it is unlikely to be unset) because resetting it would break things. That said, |
|
I agree, whether a setup is temporary is beyond our scope. We'll just want to make sure we only ever run the migration once, which seems to be covered here. And minize damage if something trips up while we're migrating. I'll need to look a closer look at this all though. |
|
Just for the record: I don't really like this auto-migration. I would be more happy if rpm just checks the new location first and maybe writes a warning if it finds a file in both locations. |
|
@mlschroe, any specific concerns for .. specific situations or such? |
|
Not really. I just dislike such automatic config file migration, as it tends to surprise the users. Do any other tools do this? But feel free to ignore me ;-) |
|
It's a bit of a mixed bag, but it's more common these days to do configuration migration since users wind up being surprised when you don't and they never actually do anything and it breaks when the old path/format/etc. is dropped. |
|
I don't want to ignore concerns over automatic migrations, I have my own reservations too. My thinking here is that since we place a symlink on the original file's location, we can avoid an annoying nagging for almost every rpm(build) user out there without affecting their lives significantly. Without the symlink, I wouldn't dream of such a change. I edit my ~/.rpmmacros so often, I find the new XDG-path nothing but annoying - for that purpose. It has its own benefits of course. I have been wondering if it makes sense to perform the migration on any type of rpm usage or should it be limited to some types of usages only - but then what would those be? Couple of initial remarks on the patch itself:
|
|
OK, cleaning up is a bit of a mess (no pun intended). The new code is pretty ugly but as we need to undo both files if one fails and remove the directory afterwards. So I just intertwined both operations manually. I don't think we really need a flag for failed migrations. The thing that would make this fail in an annoying ways is if the home dir was read-only. Good luck with adding a flag then... |
|
Read-only home dir is not an issue at all: the migration-in-progress flag file creation fails, and thus the migration never starts at all. Actually the flag file probably simplifies the cleanup code because we need to guess less about the circumstances on cleanup. Plus, the presence of the flagfile is useful evidence for post-mortem, should something go wrong (like, power-outage) It also occurred to me like at some point over the weekend (but no I didn't think about this all weekend 😆 ): we should have that migration flag file to protect against parallel execution, which could otherwise go sideways in various interesting ways if an unsuspecting user runs multiple rpm instances simultaneously. Chances for that might not be high but we've been seeing these kind of issues filed increasingly in recent years. The bigger question is whether to do it automatically or not, and that's up to discussion I think. The motivation for wanting fully automatic migration was to be able to rely on the XDG directory presence for storing auto-signing keys (so #2678 and #3522) in an rpm-specific location. Since this wasn't done at that point, I needed to do it differently, and so (one of) the main motivation for auto-migration is no longer there. I'm now in this endless loop of:
Thoughts welcome 😄 We'll keep the migration code in C++ anyway so you can go ahead with the flag file thing regardless of the manual/auto decision though. |
|
To be honest, I think that having the flag file resolves any potential "migration loop" issues only if we do the symlink thing. Is there a reason to create the symlink to the old paths? |
|
We couldn't even dream of automatic migration without preserving the original paths. That said, @mcurlej pointed out last week there's another option for doing the automatic migration: leave the original files in place as-is, and symlink from ~/.config/rpm to them. It's less error-prone and less intrusive. It's also not much of a migration, except to guarantee we always have that XDG dir, but then that was the main motivation behind this to begin with. Only, it no longer matters as much. If the migration is manually invoked with some --migrateconfig switch or such, it needs a whole lot less guarding. |
|
Well, if it's manually invoked, we can ship a systemd user unit to do it on first login into an upgraded system. |
This is effectively a pointless migration. We should not do it this way because then it makes migration to XDG directories useless since the data is still in the old location and none of the tools can detect that it should move to the new location. |
|
Such a migration would not be useless at all if it helps to ensure that we can rely on having ~/.config/rpm around. Which was the main reason we started looking into the (semi-)automatic migration. I don't care about that option too much either because it doesn't really migrate, but it has it's own benefits. Just a slightly different compromise. The comment about systemd unit to convert had me thinking: no matter what the mechanism, I guess any automated migration would need to be limited to interactive sessions - ie at least have a tty. Scripted setups is where the more exotic things tend to live, and it's just best to just leave them to be manually taken care of. |
There was a problem hiding this comment.
I was quite close to squashing + merging but, toying around with this on my local system a bit reveals a nasty bug:
if ~/.rpmmacros migration succeeds but ~/.rpmrc fails, the macros file is *lost entirely - ~/.config/rpm is nuked, and ~/.rpmmacros is only a broken symlink.
An easy way to force the above scenario is 'chattr +i ~/.rpmmrc'.
It's the good old "the case you don't test is the one with the bugs" 😅
|
It's probably better to only bother creating the symlinks after both files have been successfully moved to the new location, there's less to undo in that case. One other unrelated thing that occurred to me is that we might want to skip the migration for root. Not so much that root's ~/.rpm* files are somehow special but to make sure we don't accidentally move some users files when running as root due to environment mixups. |
|
Oh, one more thing that is completely missing here: documentation. The migration behavior, along with the lock file it leaves in case of failure, needs to be documented. rpm-config(5) is the place for that as it describes the overall initialization and paths. |
What is super annoying about this is that I had tests using |
ffesti
force-pushed
the
3467
branch
2 times, most recently
from
0f50b6c to
9ab1dd2
Compare
|
OK, as we no longer copy any files we really shouldn't delete anything but the symlinks we created. As said above the test suite doesn't like chattr (even when installing it in the container). This no longer does the migration with root privileges (test case 014 checks this en passant). Added more or less the same text to both relevant man pages. |
|
There are a few cosmetic items left still that could be addressed within a day but: I'm not going to merge a change this disruptive on the eve of the beta, stuff like this needs some real-world soak-time in our own use before unleashing to users (see the above remark on the situations where this may run). We can still merge it afterwards for that soak-time and then tweak as necessary and finally (re)consider whether we can ship it as-is as late as beta2. The file-level logistics of this seem fine now, on a quick glance. It looked okay on the previous round too though, so I'm goint to have some hands-on time with this. The biggest issue with this whole thing is the late realization (from the man page update) that as-is, this would run on anything loading initializing the rpm library. Which can run in all manner of very wild contexts, including but not limited to cgi-scripts in parallel and who knows what. I think we need to find a way to contain this to only rpm commands - of those we at least have a reasonable idea how and where they are commonly executed. Another realization from last evening is that the "root issue" is somewhat more subtle: it's not that we could never migrate root's files, it's that we can't let root migrate other users files. So basically we'd need to verify the uid's of the files we're about to move. No need to do that here, just a general remark that we could lift the root restriction by doing so, in a later patch perhaps. |
The XDG cofiguration directory is the new default location for the macros and rpmrc file. Migrate legacy files automatically. Create symlinks from the home directory for compatibility with older rpm versions. Don't migrate for more complicated setups, if running with root privileges or in scripts. Leaves ~/.rpmconfig_migration_lock_ behind if the migration fails and attempt no more migrations. Resolves: rpm-software-management#3467
|
Having slept over it, I think the best way to tame the situations where the migration may happen to something manageable is hooking it to programs that run rpmcliInit() / rpmcliFini(). There's even a variable that tracks the the thing we need already, all you need is something like ...and then check for that as the first condition when considering the migration. And then it should be safe enough to be merged. I'm calling it rpmcliIsConfigured() instead of somethinginitialized because there's already rpmcliConfigured() that you call if you want to ensure cli has been initialized, and this new function pairs with that: check if it's initialized without initializing as a side-effect. |
|
Tagging @dmnks for the remaining review as I'm off to vacation. I sure hope to see this done & merged before I return. |
|
I'm a bit late to the party but here's my thoughts: This seems like a bad idea to me. The amount of code this requires, plus the risk of missing one of those corner cases (did we really consider them all?), are all red flags for me. I'm of the opinion that one's There's also one scenario which I don't see mentioned here, which is managing one's dotfiles with something like GNU Stow (which I personally do). In such a case, I'd already have a symlink at Yes, it would be nice if we could rely on there being a Currently, we fall back to the legacy locations completely if just one of those files exists. Couldn't we make this fallback per-file instead? That way, no migration would be required and we could still benefit from having |
I stand corrected - having actually looked at this patch, it does bail out if the old files aren't regular files, which means if one has a custom symlink such as |
| free(userrc); | ||
| usermacros = xstrdup(oldmacros); | ||
| userrc = xstrdup(oldrc); | ||
| if ((rpmGlob(oldmacros, NULL, NULL) == 0 || rpmGlob(oldrc, NULL, NULL) == 0)) { |
There was a problem hiding this comment.
This line change seems accidental (?)
| []) | ||
| RPMTEST_CLEANUP | ||
|
|
||
| RPMTEST_SETUP_RW([macro path]) |
There was a problem hiding this comment.
There already is a test with this name, maybe add something after a colon?
| RPMTEST_CLEANUP | ||
|
|
||
| RPMTEST_SETUP_RW([macro path]) | ||
| AT_KEYWORDS([convert .macros]) |
There was a problem hiding this comment.
This, OTOH, should probably be just macros, like in the other tests.
| fs::remove(userdir_path, ec2); | ||
| err: | ||
| if (fs::exists(oldmacros, ec2)) { | ||
| rpmlog(RPMLOG_ERR, "Could not migrate %s to %s: %s\n", |
There was a problem hiding this comment.
Do we really want this to be an error? It seems too drastic, a failed migration doesn't mean reduced functionality in rpm, it's more of a house-keeping thing that's nice to have (at least for now).
| runroot_user rpm -E "%{foo}" | ||
| runroot_user chmod 755 /home/$RPMUSER/.config | ||
| runroot_user ls /home/$RPMUSER/.rpmconfig_migration_lock | ||
| runroot_user rpm -E "%{foo}" |
There was a problem hiding this comment.
This probably deserves a readlink check, like those in the subsequent test, just to be sure. It is kinda covered by the lack of a warning: Migrated ... message in the output, but that's relying on the message alone.
| rpm --showrc | awk '/^Macro path/{print(a[split($0,a,":")])}' | ||
| runroot rm -rf /root/.config/rpm /root/.rpmmacros | ||
| runroot touch /root/.rpmmacros | ||
| runroot rpm --showrc | awk '/^Macro path/{print(a[split($0,a,":")])}' |
There was a problem hiding this comment.
Are these changes intentional?
| does not exist. | ||
| In older versions of rpm, the path of per-user macros was | ||
| _~/.rpmmacros_. RPM will try to move this file to the new | ||
| locations. If this is not possible it will create |
There was a problem hiding this comment.
Maybe mention that XDG_CONFIG_HOME is the new location.
| _~/.rpmmacros_. RPM will try to move this file to the new | ||
| locations. If this is not possible it will create | ||
| _~/.rpmconfig_migration_lock_ to prevent attempting the migration over | ||
| and over again. In this case the file is still processed if it exists |
There was a problem hiding this comment.
I suppose "this file" is the macros file, correct? If so, I'd suggest mentioning it explicitly here, to avoid confusion.
Also, I wonder if we should mention here (briefly) what happens when the lock file is removed. As it is now, the migration would only be retried if the new directory in XDG_CONFIG_HOME doesn't exist. That means, a borked migration would have to be fixed manually by the user.
| if (!fs::is_regular_file(oldmacros, ec) && !fs::is_regular_file(oldrpmrc, ec)) | ||
| return -1; | ||
|
|
||
| int fd = open(lockfile.c_str(), O_CREAT|O_EXCL|O_WRONLY, 0644); |
There was a problem hiding this comment.
Perhaps silly, but couldn't we reuse the rpmlock mechanism here? That said, I'm not familiar with the inner workings of it myself, so not sure it's a good fit, just something that comes to mind.
5 participants
The XDG cofiguration directory is the new default location for the macros and rpmrc file. Migrate legacy files automatically. Create symlinks from the home directory for compatibility with older rpm versions.
Resolves: #3467