[Rpm-maint] [rpm-software-management/rpm] Move ~/.rpmmacros and ~/.rpmrc to ~/.config/rpm/ (PR #3750)

[Michal Domonkos]

@dmnks commented on this pull request.

I've looked through the patch and added some inline comments. Note that this isn't an "ack" for the whole approach, my previous concerns still apply, but I wanted to have a closer look at the logic in case we do decide to go with it.

> @@ -379,11 +485,13 @@ static void setDefaults(void)
     if (rpmGlob(userdir, NULL, NULL)) {
 	const char *oldmacros = "~/.rpmmacros";
 	const char *oldrc = "~/.rpmrc";
-	if (rpmGlob(oldmacros, NULL, NULL) == 0 || rpmGlob(oldrc, NULL, NULL) == 0) {
-	    free(usermacros);
-	    free(userrc);
-	    usermacros = xstrdup(oldmacros);
-	    userrc = xstrdup(oldrc);
+	if ((rpmGlob(oldmacros, NULL, NULL) == 0 || rpmGlob(oldrc, NULL, NULL) == 0)) {

This line change seems accidental (?)

> @@ -50,6 +50,60 @@ SOMENV=/tmp/somewhere rpm --macros "%{getenv:SOMENV}" --eval "%somewhere"
 [])
 RPMTEST_CLEANUP
 
+RPMTEST_SETUP_RW([macro path])

There already is a test with this name, maybe add something after a colon?

> @@ -50,6 +50,60 @@ SOMENV=/tmp/somewhere rpm --macros "%{getenv:SOMENV}" --eval "%somewhere"
 [])
 RPMTEST_CLEANUP
 
+RPMTEST_SETUP_RW([macro path])
+AT_KEYWORDS([convert .macros])

This, OTOH, should probably be just `macros`, like in the other tests.

> +    return 0;
+undo:
+    if (move_macros && fs::is_symlink(oldmacros, ec2))
+	fs::remove(oldmacros, ec2);
+    if (move_rpmrc && fs::is_symlink(oldrpmrc, ec2))
+	fs::remove(oldrpmrc, ec2);
+
+    if (fs::is_regular_file(newmacros, ec2))
+	fs::rename(newmacros, oldmacros, ec2);
+    if (fs::is_regular_file(newrpmrc, ec2))
+	fs::rename(newrpmrc, oldrpmrc, ec2);
+    /* userdir_path did not exist at the beginning */
+    fs::remove(userdir_path, ec2);
+err:
+    if (fs::exists(oldmacros, ec2)) {
+	rpmlog(RPMLOG_ERR, "Could not migrate %s to %s: %s\n",

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).

> +AT_KEYWORDS([convert .macros])
+RPMTEST_USER
+
+runroot_user rm -f /home/$RPMUSER/.config/rpm
+runroot_user rm -f /home/$RPMUSER/.rpmmacros
+runroot_user rm -f /home/$RPMUSER/.rpmrc
+runroot_user bash -c "echo %foo bar > /home/$RPMUSER/.rpmmacros"
+runroot_user touch "/home/$RPMUSER/.rpmrc"
+
+RPMTEST_CHECK([[
+runroot_user mkdir /home/$RPMUSER/.config
+runroot_user chmod 000 /home/$RPMUSER/.config
+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}"

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.

> @@ -7,9 +7,9 @@ AT_KEYWORDS([macros])
 
 # .rpmmacros exists, new directory not
 RPMTEST_CHECK([[
-rm -rf ~/.config/rpm ~/.rpmmacros
-touch ~/.rpmmacros
-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,":")])}'

Are these changes intentional?

> @@ -38,9 +38,12 @@ _macro path_ uses this to achieve the following hierarchy of settings:
 
 The default _macro path_ can be inspected with *rpm --showrc|grep ^Macro*.
 
-In older versions of rpm, the path of per-user macros was _~/.rpmmacros_.
-This is still processed if it exists and the new configuration directory
-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

Maybe mention that `XDG_CONFIG_HOME` is the new location.

> @@ -38,9 +38,12 @@ _macro path_ uses this to achieve the following hierarchy of settings:
 
 The default _macro path_ can be inspected with *rpm --showrc|grep ^Macro*.
 
-In older versions of rpm, the path of per-user macros was _~/.rpmmacros_.
-This is still processed if it exists and the new configuration directory
-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
+_~/.rpmconfig_migration_lock_ to prevent attempting the migration over
+and over again. In this case the file is still processed if it exists

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.

> +    fs::path rpmrc_target = "";
+
+    bool move_rpmrc = false;
+    bool move_macros = false;
+
+    if (userdir.substr(0, 2) == "~/")
+	userdir.replace(0, 1, home);
+
+    fs::path userdir_path = userdir;
+    fs::path newmacros = userdir_path / "macros";
+    fs::path newrpmrc = userdir_path / "rpmrc";
+
+    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);

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.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3750#pullrequestreview-3046543038
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/3750/review/3046543038 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20250723/aa7f48d4/attachment-0001.htm>