apps: Consider ID_LIKE for mapping AppStream data packages #19283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@leomoty do you mind testing this? It should fix metapackage installation on Rocky with the Refresh button for you. |
2 tasks
Yep, works as intended, albeit we might need to double check the text later, for me it just flickered for about a second. Could be a side effect of my CSS changes in the previous PR. |
mvollmer
reviewed
Sep 6, 2023
martinpitt
force-pushed
the
apps-os-mapping
branch
from
9aa513a
to
205e1ce
Compare
martinpitt
commented
Sep 6, 2023
Comment on lines
+109
to
+111
| if (val[c]) { | ||
| val = val[c]; | ||
| return true; |
There was a problem hiding this comment.
Added a test to cover this.
martinpitt
force-pushed
the
apps-os-mapping
branch
from
205e1ce
to
66756c5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
martinpitt
force-pushed
the
apps-os-mapping
branch
2 times, most recently
from
d006eb6
to
4dc8ffc
Compare
mvollmer
requested changes
Sep 7, 2023
If the daemon crashes and goes into a "failed" state, it breaks all subsequent tests. Avoid that.
Use f-strings, and drop the unnecessary (and possibly dangerous) `rm -r` option on restoration. This function already does not work for directories.
With only looking at aos-release's `ID` field, we are missing out a lot of derivatives, such as CentOS Stream, Rocky, or Debian-likes. Consider ID_LIKE as well to fix that. E.g. on Ubuntu, `ID_LIKE` is "debian", on CentOS it's "rhel fedora", on Rocky Linux it's "rhel centos fedora". Use that to clean up the manifest map, as Ubuntu and RHEL are now redundant. testBasic covers the "direct package name" case in the manifest. Add a new testOsMap test to check the distroname → packagename map that we use in real life.
martinpitt
force-pushed
the
apps-os-mapping
branch
from
4dc8ffc
to
97d8125
Compare
mvollmer
approved these changes
Sep 7, 2023
cockpituous
reviewed
Sep 7, 2023
| function get_config(name, distro_id, def) { | ||
| function get_config(name, os_release, def) { | ||
| // ID is a single value, ID_LIKE is a list | ||
| const os_list = [os_release.ID || "", ...(os_release.ID_LIKE || "").split(/\s+/)]; |
There was a problem hiding this comment.
This added line is not executed by any test. Details
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With only looking at aos-release's
IDfield, we are missing out a lot of derivatives, such as CentOS Stream, Rocky, or Debian-likes. Consider ID_LIKE as well to fix that.E.g. on Ubuntu,
ID_LIKEis "debian", on CentOS it's "rhel fedora", on Rocky Linux it's "rhel centos fedora".Use that to clean up the manifest map, as Ubuntu and RHEL are now redundant.
Spotted in #19281 (comment)