core: handle shared files and multilib #1227

jlebon · 2018-02-01T14:06:14Z

We basically do a light-weight version of what librpm does here to determine which files to really remove/install during a transaction.

Closes: #1217
Closes: #1145

Requires: ostreedev/ostree#1441

jlebon · 2018-02-01T14:07:04Z

Marking as WIP for now. There's some things to split out and minor issues to iron out on CentOS. And tests.

jlebon · 2018-02-01T14:12:13Z

src/libpriv/rpmostree-core.c

@@ -2560,20 +2736,8 @@ delete_package_from_root (RpmOstreeContext *self,
      if (!g_str_has_prefix (fn, "usr/"))
        continue;

-      /* avoiding the stat syscall is worth a bit of userspace computation */
-      if (rpmostree_str_has_prefix_in_ptrarray (fn, deleted_dirs))
-        continue;


I dropped this optimization here since we now do things like librpm does: we traverse the file list backwards, which means directories are emptied out before we delete them.

This is analogous to the filtering support for the commit API: we allow library users to skip over checking out specific files. This is useful in some tricky situations where we *know* that the files to be checked out will conflict with existing files in subtle ways. One such example is in rpm-ostree support for multilib. There, we want to allow checking out a package onto an existing tree, but skipping over files that are not coloured to our preferred value (e.g. not overwriting an i686 version of `ldconfig` if we already have the `x86_64` version). See coreos/rpm-ostree#1227 for details.

cgwalters · 2018-02-05T17:19:52Z

This really ramps up quite significantly the complexity around doing checkouts. I don't have any clever ideas for solving the file coloring problem without that, but it's unfortunate.

One random example, I'd been thinking about multi-threading checkouts and we'd have to evaluate doing that a lot more carefully with app callbacks involved.

OK here's a random idea: what if we we ordered the package set by "primary arch", then "other" and did checkouts of "other" with ADD_FILES? That wouldn't handle a package with mixed arches, but do those exist?

jlebon · 2018-02-05T19:37:00Z

OK here's a random idea: what if we we ordered the package set by "primary arch", then "other" and did checkouts of "other" with ADD_FILES? That wouldn't handle a package with mixed arches, but do those exist?

Hmm, not sure. I don't have a good grasp on what kinds of third-party packages are out there. :) Otherwise, I think that might work... Actually, this won't work if we're overlaying a "primary" arch when the "other" arch is the one baked in, right?

More generally though, this means we're turning off conflict detection for the whole package if only a single file is of a different colour. (Or just fall back to librpm's detection in that case, I guess?)

One random example, I'd been thinking about multi-threading checkouts and we'd have to evaluate doing that a lot more carefully with app callbacks involved.

Yeah, that makes sense; it makes things trickier. OTOH, filtering during checkout would allow us to do more sophisticated things down the line. One example that immediately comes to mind is the RPM caching problem in unified core mode. We wouldn't have to invalidate the whole cache if we changed e.g. documentation or install-langs between composes, right?

This is analogous to the filtering support for the commit API: we allow library users to skip over checking out specific files. This is useful in some tricky situations where we *know* that the files to be checked out will conflict with existing files in subtle ways. One such example is in rpm-ostree support for multilib. There, we want to allow checking out a package onto an existing tree, but skipping over files that are not coloured to our preferred value (e.g. not overwriting an i686 version of `ldconfig` if we already have the `x86_64` version). See coreos/rpm-ostree#1227 for details. Closes: #1441 Approved by: cgwalters

Split out to reduce noise in upcoming patch.

This is useful to have in a more global location since we deal with RPM and OSTree paths in various places.

Not all files from an RPM are necessarily removed during pkg erasure. For example, files which are shared between pkgs shouldn't be deleted. Similarly, not all files in an RPM are necessarily copied during pkg installs. This is the case for multilib handling, which is a mess in its own right. But such is the cost of trying to replace major parts of a long-standing foundational project like RPM. This patch adds some smarts to the way we do overlays and overrides to handle these cases by calculating beforehand which files we *should't* checkout/delete. Closes: coreos#1217 Closes: coreos#1145

jlebon · 2018-02-07T21:56:28Z

Now working on CentOS and with tests! ⬆️

cgwalters

Overall...looks pretty good. I certainly believe you've sucessfully
reverse-engineered all the file color bits. The comments are really helpful.

Tests look sane.

One thing that had me really pausing about this before was I thought
we'd need to do a lot of computation in the filter, but in fact it's
just a hash table lookup. (So in the future it could be safely multithreaded)

I also like that we don't need to have hash table entries for all files,
just the exceptions.

Still though, I had to scan through handle_file_dispositions() a few times to
really feel like I understood it. It's complex, but it's solving a complex
problem.

I wonder if somehow we could only involve this if we detect multi-colored packages in the transaction?

cgwalters · 2018-02-05T17:08:34Z

src/libpriv/rpmostree-core.c

+          const char *fn = rpmfiFN (fi);
+          g_assert (fn != NULL);
+
+          if (g_str_has_suffix (fn, "ldconfig"))


Let's remember to delete this?

cgwalters · 2018-02-08T02:29:21Z

src/libpriv/rpmostree-util.c

@@ -234,6 +234,23 @@ rpmostree_pkg_get_local_path (DnfPackage *pkg)
    }
 }

+char*
+rpmostree_translate_path_for_ostree (const char *path)


I think this one deserves at least a quick comment at the top like:
/* Convert a "traditional" path (normally from e.g. an RPM) into its final location in ostree */

cgwalters · 2018-02-08T02:40:01Z

src/libpriv/rpmostree-core.c

+
+          /* let's make the safe assumption that the color mess is only an issue for /usr */
+          const char *fn_rel = fn;
+          fn_rel += strspn (fn, "/");


Why not const char *fn_rel = fn + strspn (fn, "/")?

cgwalters · 2018-02-08T02:51:58Z

src/libpriv/rpmostree-core.c

+  /* this is a bit awkward; we relativize for the translation, but then make it absolute
+   * again to match libostree */
+  const char *path_rel = path;
+  path_rel += strspn (path, "/");


This could just be path += strspn (path, "/") right?

cgwalters · 2018-02-08T02:52:46Z

src/libpriv/rpmostree-core.c

@@ -2378,6 +2378,37 @@ rpmostree_context_consume_package (RpmOstreeContext  *self,
  return TRUE;
 }

+static void


/* Build a mapping from filename → rpm "color" */

cgwalters · 2018-02-08T03:04:05Z

src/libpriv/rpmostree-core.c

+          if (color && other_color && (color != other_color))
+            {
+              /* do we already have the preferred color installed? */
+              if (color & ts_prefcolor)


Just for my own understanding...could this be == instead of &?

Yes, you're correct AFAIU. They're both bitfields, but ts_color contains all the colors we're compatible with, while ts_prefcolor contains only a single color from that set. And files can only one specific color, so == would work equally well here.

cgwalters · 2018-02-08T03:06:43Z

src/libpriv/rpmostree-core.c

+  GHashTable *files_skip = user_data;
+  if (g_hash_table_contains (files_skip, path))
+    return OSTREE_REPO_CHECKOUT_FILTER_SKIP;
+  /* hack for nsswitch.conf, which we modify during treecompose; a better heuristic here


I don't understand why this is necessary now; what would break without this?

I expanded on the comment a bit to clarify.

jlebon · 2018-02-08T21:09:56Z

Fixups! ⬆️

cgwalters · 2018-02-08T21:55:45Z

@rh-atomic-bot r+ 81621c8

rh-atomic-bot · 2018-02-08T21:58:58Z

⚡ Test exempted: pull fully rebased and already tested.

This is useful to have in a more global location since we deal with RPM and OSTree paths in various places. Closes: #1227 Approved by: cgwalters

Not all files from an RPM are necessarily removed during pkg erasure. For example, files which are shared between pkgs shouldn't be deleted. Similarly, not all files in an RPM are necessarily copied during pkg installs. This is the case for multilib handling, which is a mess in its own right. But such is the cost of trying to replace major parts of a long-standing foundational project like RPM. This patch adds some smarts to the way we do overlays and overrides to handle these cases by calculating beforehand which files we *should't* checkout/delete. Closes: #1217 Closes: #1145 Closes: #1227 Approved by: cgwalters

jlebon mentioned this pull request Feb 1, 2018

lib/checkout: support filtering ostreedev/ostree#1441

Closed

jlebon added the WIP label Feb 1, 2018

jlebon commented Feb 1, 2018

View reviewed changes

jlebon added 3 commits February 7, 2018 21:55

core: move utility function higher up

6005c2b

Split out to reduce noise in upcoming patch.

libpriv/importer: factor out pathname translator

01a1e02

This is useful to have in a more global location since we deal with RPM and OSTree paths in various places.

jlebon force-pushed the pr/coloring branch from 62dec38 to a6926e0 Compare February 7, 2018 21:56

jlebon removed the WIP label Feb 7, 2018

jlebon changed the title ~~WIP: calculate file dispositions~~ core: handle shared files and multilib Feb 7, 2018

kladiv mentioned this pull request Feb 8, 2018

Handle multilib (file coloring) #1217

Closed

cgwalters reviewed Feb 8, 2018

View reviewed changes

jlebon added 2 commits February 8, 2018 21:07

fixup! core: handle shared files and multilib

b64afe5

fixup! libpriv/importer: factor out pathname translator

81621c8

cgwalters added the reviewed/approved label Feb 8, 2018

rh-atomic-bot closed this in d6b04a0 Feb 8, 2018

rh-atomic-bot pushed a commit that referenced this pull request Feb 8, 2018

libpriv/importer: factor out pathname translator

9a4d780

This is useful to have in a more global location since we deal with RPM and OSTree paths in various places. Closes: #1227 Approved by: cgwalters

jlebon deleted the pr/coloring branch February 9, 2018 21:00

jlebon mentioned this pull request Apr 19, 2018

override replace with systemd - missing unit files #1340

Closed

jlebon mentioned this pull request Apr 22, 2020

remove-from-packages: deleting files it shouldn't #2068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: handle shared files and multilib #1227

core: handle shared files and multilib #1227

jlebon commented Feb 1, 2018

jlebon commented Feb 1, 2018

jlebon Feb 1, 2018

cgwalters commented Feb 5, 2018

jlebon commented Feb 5, 2018

jlebon commented Feb 7, 2018

cgwalters left a comment

cgwalters Feb 5, 2018

cgwalters Feb 8, 2018

cgwalters Feb 8, 2018

cgwalters Feb 8, 2018

cgwalters Feb 8, 2018

cgwalters Feb 8, 2018

jlebon Feb 8, 2018

cgwalters Feb 8, 2018

jlebon Feb 8, 2018

jlebon commented Feb 8, 2018

cgwalters commented Feb 8, 2018

rh-atomic-bot commented Feb 8, 2018

core: handle shared files and multilib #1227

core: handle shared files and multilib #1227

Conversation

jlebon commented Feb 1, 2018

jlebon commented Feb 1, 2018

Choose a reason for hiding this comment

cgwalters commented Feb 5, 2018

jlebon commented Feb 5, 2018

jlebon commented Feb 7, 2018

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Feb 8, 2018

cgwalters commented Feb 8, 2018

rh-atomic-bot commented Feb 8, 2018