qmerge: Add .xpak binpkg-multi-instance support #16

genbtc · 2022-05-02T23:02:42Z

The current implementation of Qmerge did not support FEATURES="binpkg-multi-instance" directory structures suitable for .xpak style packages. (ie: one subdirectory for each package, with package names of ATOM-BUILD_ID + .xpak extension).

old .tbz2 directory example:
/var/cache/binpkgs/
/var/cache/binpkgs/dev-util
/var/cache/binpkgs/dev-util/desktop-file-utils-0.26-r2.tbz2

binpkg-multi-instance Example:
/var/cache/binpkgs/
/var/cache/binpkgs/dev-libs
/var/cache/binpkgs/dev-libs/libpwquality
/var/cache/binpkgs/dev-libs/libpwquality/libpwquality-1.4.4-3.xpak
/var/cache/binpkgs/dev-libs/libpwquality/libpwquality-1.4.4-4.xpak

The logic was already in place to actually read, interpret, and extract these xpak files, BUT it did not know how to find them. Resulting in errors:

# q merge -pO libpwquality
[R] dev-libs/libpwquality-1.4.4
qmerge: /var/cache/binpkgs/dev-libs/libpwquality-1.4.4.tbz2 appears not to be a valid tbz2 file

This patch adds support for the multi-dir structure, based on reading the /var/cache/binpkgs/Packages index cache store file for the particular Atom, detecting if the "PATH" variable exists in the index (it would not exist for an old .tbz2), then following this exact PATH to locate the binpkg instead of the old way of attempting to guess and reconstruct a path from its atom sub-components (since it would be impossible to know the BUILD_ID this way).

(This patch relies on using the Packages file to read PATH)
CPV: dev-libs/libpwquality-1.4.4
PATH: dev-libs/libpwquality/libpwquality-1.4.4-3.xpak

I have tested it both with and without binpkg-multi-instance, and it works for both, and a combination of the two. The existing tests/ scripts all seem to still pass.

I added a warning message to the qmerge_main() function to detect the FEATURES, but this detection is not tied to any actual logic currently. I just wanted a message so people take note, in case the patch is non-functional due to some edge case overlooked, for example there are still some inadequacies and inconsistencies:

1 - always chooses the first match, instead of choosing the highest number BUILD_ID.
(in my example it chooses -3.xpak instead of -4.xpak).
2 - just doing a FS directory walk to find any .xpak's that way, instead of using the Packages cache file is not implemented.
3 - Also broken is binhost fetch -f or -F download support from a multi-instance target - Bug #654608 - but that is substantially more problematic.

I have also added some extra Q_ metadata variables that were as yet unaccounted for, that we can cache from the Packages index file format. Just because it took me several days to understand why PATH was not being read, how to even do so, and having other variables already exist and set up could save a new contributor some time, and since the ABI of "libq" was affected by adding PATH anyway.

Thank you. I Will be available on gentoo IRC as genr8<tabcomplete> to discuss, and I think I can be of use working on this important tool.

grobian

Nice to see such contribution. I think we'll have to look into if we can actually make it select a most recent build id or something, but that would depend on the Packages file, I believe.

grobian · 2022-05-03T06:36:03Z

libq/tree.c

@@ -1483,6 +1483,7 @@ tree_foreach_packages(tree_ctx *ctx, tree_pkg_cb callback, void *priv)
 		if (strcmp(p, "REPO") == 0) { /* from global section in older files */
 			ctx->repo = c;
 		} else if (strcmp(p, "CPV") == 0) {
+			meta.Q_CPV = c;


why do you need this? it should be part of atom

grobian · 2022-05-03T06:36:38Z

libq/tree.c

@@ -1510,6 +1511,15 @@ tree_foreach_packages(tree_ctx *ctx, tree_pkg_cb callback, void *priv)
 		match_key(PDEPEND);
 		match_key2(REPO, repository);
 		match_key(SIZE);
+		match_key(BDEPEND);
+		//


please don't use //
any reason there is flagging here?

grobian · 2022-05-03T06:38:16Z

libq/tree.h

@@ -99,6 +99,13 @@ struct tree_pkg_meta {
 	char *Q_PROPERTIES;
 	char *Q_BDEPEND;
 	/* binpkgs/vdb */
+	//


remove these markers for the final commit please

grobian · 2022-05-03T06:40:31Z

qmerge.c

@@ -2025,6 +2026,12 @@ int qmerge_main(int argc, char **argv)

 	qmerge_strict = contains_set("strict", features) ? 1 : 0;

+	binpkg_multi_instance = contains_set("binpkg-multi-instance", features) ? 1 : 0;
+	if (binpkg_multi_instance) {


Nice to be careful, but let's not do this. If you insist, pair it with verbose > 1 or something

grobian · 2022-05-03T06:42:55Z

libq/tree.h

+	char *Q_PATH;
+	char *Q_BUILD_ID;
+	char *Q_BUILD_TIME;
+	char *Q_REQUIRES;


from all of these, you only use Q_PATH

I know you wanted to have the complete set so others don't have to look for it, but I rather save memory and not have pointers for keys that aren't ever used

i have made the requested changes. but I plan on using the BUILD_ID and BUILD_TIME next, they are directly relevant to picking the right multiple version.

Do you want to make all of these available, or just the latest? The problem you'll be facing is that the version remains the same (adding the build id would make the atom invalid). If you only want to keep the latest, you simply have to replace the previous one, if you want to expose all versions, you'll need a hack so atom_sort will be able to order them somehow. This will get ugly quickly I think.

im not 100% sure what the question means, but I can say that is basically why i havent done that part yet. however, choosing any build_ID version is better than none, so i think this should be committed first.

binpkg-multi-instance has become the default in new installs since Bug #833582 was fixed for good 2 weeks ago, and the majority 90%? use case is most people will have 1 build per version. so it should be fixed soon so qmerge can work again.

after that, only the latest build_ID could be chosen by default instead, and after that, we can work on people selecting a build_ID version as part of the user input atom matching. Portage knows how to do this, so we could read that and find out how. I understand the function structure of tree.c does currently not lend well but nothings impossible.

Ok, I'd like to go as far as just selecting the highest build_id, would that make sense? Lacking any specification of this format (I filed a bug for that long ago) I'm assumign the build_id is a number and we can just numerically compare it to the highest version, right?

grobian · 2022-05-19T07:44:48Z

observation: the -<BUILDID> suffix used is not conformant to PMS, which means that atom_explode cannot handle it (nor can it be made to handle it, for it is ambigious whether it would be a build id or not, PMS is clear about that, as package names may include -<number>).

So, BUILDID, what is it? Much like EAPI, I guess while technically it can be a string, realistically it's going to be an integer. So that integer can be added to libq/atom.h much like PR_int. BUILDID_int perhaps? Not sure about the _int suffix. It means we just add 4 bytes to every atom.

References: #16 Signed-off-by: Fabian Groffen <grobian@gentoo.org>

grobian · 2022-05-19T07:48:31Z

In 74aaef6 I added BUILDID.

Now the problem is that atom_explode can't populate it. So the code in tree that is walking the Packages file, should extract the BUILD_ID, convert it to an integer, and set it in the BUILDID member of the atom made from CPV.

This way, atom_compare can use it to order the various builds for the same version.

PR: #16 Signed-off-by: Fabian Groffen <grobian@gentoo.org>

grobian · 2022-05-19T08:17:42Z

In be7090b I added support to atom_compare to use BUILDID, which should more or less make your code in tree automatically select the highest BUILDID, should you populate BUILDID in the atom.

qmerge: Add .xpak binpkg-multi-instance support

a08e970

thesamesam requested a review from grobian May 3, 2022 00:59

grobian reviewed May 3, 2022

View reviewed changes

make requested changes

cadca67

genbtc force-pushed the main branch from 639ced9 to cadca67 Compare May 4, 2022 03:19

gentoo-bot pushed a commit that referenced this pull request May 19, 2022

libq/atom.h: add BUILDID to atom for binpkg-multi-instance support

74aaef6

References: #16 Signed-off-by: Fabian Groffen <grobian@gentoo.org>

gentoo-bot pushed a commit that referenced this pull request May 19, 2022

libq/atom: make atom_compare consider BUILDID

be7090b

PR: #16 Signed-off-by: Fabian Groffen <grobian@gentoo.org>

gentoo-bot closed this in b868d22 May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qmerge: Add .xpak binpkg-multi-instance support #16

qmerge: Add .xpak binpkg-multi-instance support #16

genbtc commented May 2, 2022 •

edited

grobian left a comment

grobian May 3, 2022

grobian May 3, 2022

grobian May 3, 2022

grobian May 3, 2022

grobian May 3, 2022

genbtc May 4, 2022

grobian May 4, 2022

genbtc May 4, 2022

grobian May 5, 2022

grobian commented May 19, 2022

grobian commented May 19, 2022

grobian commented May 19, 2022

qmerge: Add .xpak binpkg-multi-instance support #16

qmerge: Add .xpak binpkg-multi-instance support #16

Conversation

genbtc commented May 2, 2022 • edited

grobian 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

grobian commented May 19, 2022

grobian commented May 19, 2022

grobian commented May 19, 2022

genbtc commented May 2, 2022 •

edited