Thanks for working on this! I'll take a look.

This PR changes the package list in fedora-common-ostree.yaml for arch specific packages.
Can you make sure that running a sync after your change does not change the current list?
If the packages should be removed then we need to make that explicit or make another PR.

Ah, looking at https://gitlab.com/fedora/sigs/sway/ostree-config/-/merge_requests/4 I think I understand the change more.

It's easier to review changes with the sync result in the same commit as the change.
Can you push your changes with a sync here?
Another sync that would make this review easier is a split with a first PR that adds the code but results in no package list change and then the changes that moves packages around to per-variant lists.

It's easier to review changes with the sync result in the same commit as the change.
Can you push your changes with a sync here?
Another sync that would make this review easier is a split with a first PR that adds the code but results in no package list change and then the changes that moves packages around to per-variant lists.

Makes sense, I'll try to get that done this weekend.
I'll leave base-x changes in our gitlab branch though, as we're not ready to send a PR for Sway yet.

Couple of random thoughts as I'm struggling to dedicate review time for this one as we both don't have CI and a way to validate that the changes are a NOP for now:

If you could get the changes required in fedora-comps (I can merge them there) and libcomps (I can not but can review) then that would be great for the future

How about we make this script generate yaml manifests that include themselves like the comps groups instead of generating very verbose and duplicated lists? This might make things easier to manage and follow. With this layout, we would be reproducing the comps groups as a tree of manifests.

libcomps PR: https://github.com/rpm-software-management/libcomps/pull/92

For the fedora-comps update, what are the architectures we care about? I want to keep it minimal, so if the package is, for example, ExcludeArch: s390x, we may not even need to bother.

How about we make this script generate yaml manifests that include themselves like the comps groups instead of generating very verbose and duplicated lists? This might make things easier to manage and follow. With this layout, we would be reproducing the comps groups as a tree of manifests.

I think that is fine for core groups, but what if the KDE or Sway or anything else wants a different set of packages from, say, @input-methods? Current scheme allows to exclude packages for the specific desktop from any group that is not in the fedora-ostree-common part, do we want to preserve that?

By the way, would getting #280 in first help the review?
I only had one nit on that PR, where exclude_list = comps_desktop_exclude_list.get(group, set()) should probably be exclude_list = comps_desktop_exclude_list.get(f"{desktop}-desktop", set()) to ensure that desktop_exclude_list still works as expected.

libcomps PR: https://github.com/rpm-software-management/libcomps/pull/92

Thanks for the libcomps PR.

what are the architectures we care about?

We care about x86, aarch64 & ppc64le for Silverblue & Kinoite.

I think that is fine for core groups, but what if the KDE or Sway or anything else wants a different set of packages from, say, @input-methods? Current scheme allows to exclude packages for the specific desktop from any group that is not in the fedora-ostree-common part, do we want to preserve that?

Good question. I'm not sure this happens that often.

I'm not sure #280 is the right approach.

I didn't see this before I wrote mine, but my #312 handles the arch problem here in, I think, a better way.

BTW, as of https://pagure.io/fedora-comps/pull-request/767 , fedora-comps should be 100% 'correct' regarding non-existent packages and arches. I'm re-running the check script to see if any new problems appeared in the last two months. edit: looks like there's a handful of new retirements, I'll fix those.

I'm not sure this is any use now, since my #312 was merged. I'm pretty sure that addresses the problems.

We still need the functionality from #280, but at this point I'll just wait for @siosm to complete it instead of trying to salvage this PR. Or work around it in a very ugly way :)

that shouldn't be too hard, I don't think, I didn't worry about it as it didn't seem necessary for anything ATM. do you need it for some new desktop or something?