From 2f1e9a34b612e20e7c6b27efb186f6d83e04bf48 Mon Sep 17 00:00:00 2001 From: Jelle van der Waa Date: Mon, 30 Jan 2023 17:44:43 +0100 Subject: [PATCH 1/7] shell: python: load translations via manifest.js Split the po files into po.$lang.js and po.manifest.$lang.js so the shell can only serve the translated manifests for the C bridge we now need to also load globbed manifest translations. The Python bridge does not support globbing so we need to explicitly load the po.js file as well. Closes #18224 Co-authored-by: Allison Karlitskaya This is intrusive for rhel-9.3 branch, but field-tested by now, and allows us to cleanly backport other important fixes. Cherry-picked from 89781bacb1027817b --- pkg/lib/cockpit-po-plugin.js | 27 ++++++++++++++++++--------- pkg/shell/index.html | 3 +++ src/cockpit/packages.py | 31 ++++++++++++++++++++----------- test/pytest/test_packages.py | 30 ++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/pkg/lib/cockpit-po-plugin.js b/pkg/lib/cockpit-po-plugin.js index a7f31ca6466..624198d31ac 100644 --- a/pkg/lib/cockpit-po-plugin.js +++ b/pkg/lib/cockpit-po-plugin.js @@ -44,10 +44,7 @@ function get_plural_expr(statement) { return expr; } -function buildFile(po_file, subdir, webpack_module, webpack_compilation) { - if (webpack_compilation) - webpack_compilation.fileDependencies.add(po_file); - +function buildFile(po_file, subdir, webpack_module, webpack_compilation, out_path, filter) { return new Promise((resolve, reject) => { const parsed = gettext_parser.po.parse(fs.readFileSync(po_file), 'utf8'); delete parsed.translations[""][""]; // second header copy @@ -76,6 +73,9 @@ function buildFile(po_file, subdir, webpack_module, webpack_compilation) { if (translation.comments.flag?.match(/\bfuzzy\b/)) continue; + if (!references.some(filter)) + continue; + const key = JSON.stringify(context_prefix + msgid); // cockpit.js always ignores the first item chunks.push(`,\n ${key}: [\n null`); @@ -90,8 +90,6 @@ function buildFile(po_file, subdir, webpack_module, webpack_compilation) { const wrapper = config.wrapper?.(subdir) || DEFAULT_WRAPPER; const output = wrapper.replace('PO_DATA', chunks.join('')) + '\n'; - const lang = path.basename(po_file).slice(0, -3); - const out_path = (subdir ? (subdir + '/') : '') + 'po.' + lang + '.js'; if (webpack_compilation) webpack_compilation.emitAsset(out_path, new webpack_module.sources.RawSource(output)); else @@ -110,9 +108,20 @@ function init(options) { function run(webpack_module, webpack_compilation) { const promises = []; - config.subdirs.map(subdir => - promises.push(...get_po_files().map(po_file => - buildFile(po_file, subdir, webpack_module, webpack_compilation)))); + for (const subdir of config.subdirs) { + for (const po_file of get_po_files()) { + if (webpack_compilation) + webpack_compilation.fileDependencies.add(po_file); + const lang = path.basename(po_file).slice(0, -3); + promises.push(Promise.all([ + // Separate translations for the manifest.json file and normal pages + buildFile(po_file, subdir, webpack_module, webpack_compilation, + `${subdir}/po.${lang}.js`, str => !str.includes('manifest.json')), + buildFile(po_file, subdir, webpack_module, webpack_compilation, + `${subdir}/po.manifest.${lang}.js`, str => str.includes('manifest.json')) + ])); + } + } return Promise.all(promises); } diff --git a/pkg/shell/index.html b/pkg/shell/index.html index 6d09105f32b..96a76a89d50 100644 --- a/pkg/shell/index.html +++ b/pkg/shell/index.html @@ -8,7 +8,10 @@ + + +
diff --git a/src/cockpit/packages.py b/src/cockpit/packages.py index 388c40b689f..938b6203fc2 100644 --- a/src/cockpit/packages.py +++ b/src/cockpit/packages.py @@ -157,7 +157,8 @@ def __init__(self, path: Path, value: JsonObject): class Package: - PO_JS_RE: ClassVar[Pattern] = re.compile(r'po\.([^.]+)\.js(\.gz)?') + # For po{,.manifest}.js files, the interesting part is the locale name + PO_JS_RE: ClassVar[Pattern] = re.compile(r'(po|po\.manifest)\.([^.]+)\.js(\.gz)?') # immutable after __init__ manifest: Manifest @@ -166,7 +167,7 @@ class Package: priority: int # computed later - translations: Optional[Dict[str, str]] = None + translations: Optional[Dict[str, Dict[str, str]]] = None files: Optional[Dict[str, str]] = None def __init__(self, manifest: Manifest): @@ -186,7 +187,7 @@ def ensure_scanned(self) -> None: return self.files = {} - self.translations = {} + self.translations = {'po.js': {}, 'po.manifest.js': {}} for file in self.path.rglob('*'): name = str(file.relative_to(self.path)) @@ -195,14 +196,20 @@ def ensure_scanned(self) -> None: po_match = Package.PO_JS_RE.fullmatch(name) if po_match: - locale = po_match.group(1) + basename = po_match.group(1) + locale = po_match.group(2) # Accept-Language is case-insensitive and uses '-' to separate variants lower_locale = locale.lower().replace('_', '-') - self.translations[lower_locale] = name + + self.translations[f'{basename}.js'][lower_locale] = name else: basename = name[:-3] if name.endswith('.gz') else name self.files[basename] = name + # support old cockpit-po-plugin which didn't write po.manifest.??.js + if not self.translations['po.manifest.js']: + self.translations['po.manifest.js'] = self.translations['po.js'] + def get_content_security_policy(self) -> str: policy = { "default-src": "'self'", @@ -236,21 +243,21 @@ def load_file(self, filename: str) -> Document: return Document(path.open('rb'), content_type, content_encoding, content_security_policy) - def load_translation(self, locales: List[str]) -> Document: + def load_translation(self, path: str, locales: List[str]) -> Document: self.ensure_scanned() assert self.translations is not None # First check the locales that the user sent for locale in locales: with contextlib.suppress(KeyError): - return self.load_file(self.translations[locale]) + return self.load_file(self.translations[path][locale]) # Next, check the language-only versions of variant-specified locales for locale in locales: language, _, region = locale.partition('-') if region: with contextlib.suppress(KeyError): - return self.load_file(self.translations[language]) + return self.load_file(self.translations[path][language]) # We prefer to return an empty document than 404 in order to avoid # errors in the console when a translation can't be found @@ -261,9 +268,9 @@ def load_path(self, path: str, headers: JsonObject) -> Document: assert self.files is not None assert self.translations is not None - if path == 'po.js': + if path in self.translations: locales = parse_accept_language(headers) - return self.load_translation(locales) + return self.load_translation(path, locales) else: return self.load_file(self.files[path]) @@ -496,13 +503,15 @@ def load_manifests_js(self, headers: JsonObject) -> Document: for name, package in self.packages.items(): if name in ['static', 'base1']: continue + # find_translation will always find at least 'en' - translation = package.load_translation(locales) + translation = package.load_translation('po.manifest.js', locales) with translation.data: if translation.content_encoding == 'gzip': data = gzip.decompress(translation.data.read()) else: data = translation.data.read() + chunks.append(data) chunks.append(b""" diff --git a/test/pytest/test_packages.py b/test/pytest/test_packages.py index 7c61b3b21e6..8f3a8f1cd64 100644 --- a/test/pytest/test_packages.py +++ b/test/pytest/test_packages.py @@ -158,3 +158,33 @@ def test_condition_hides_priority(pkgdir): assert packages.packages['basic'].manifest['description'] == 'standard package' assert packages.packages['basic'].manifest['requires'] == {'cockpit': "42"} assert packages.packages['basic'].priority == 1 + + +def test_translation(pkgdir): + # old style: make sure po.de.js is served as fallback for manifest translations + make_package(pkgdir, 'one') + (pkgdir / 'one' / 'po.de.js').write_text('eins') + + # new style: separated translations + make_package(pkgdir, 'two') + (pkgdir / 'two' / 'po.de.js').write_text('zwei') + (pkgdir / 'two' / 'po.manifest.de.js').write_text('zwo') + + packages = Packages() + + # make sure we can read a po.js file with language fallback + document = packages.load_path('/one/po.js', {'Accept-Language': 'es, de'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'eins' + + # make sure we fall back cleanly to an empty file with correct mime + document = packages.load_path('/one/po.js', {'Accept-Language': 'es'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + # make sure the manifest translations get sent along with manifests.js + document = packages.load_path('/manifests.js', {'Accept-Language': 'de'}) + contents = document.data.read() + assert b'eins\n' in contents + assert b'zwo\n' in contents + assert b'zwei\n' not in contents From 91d9364ce60f1bf5c8d679e656303a467cb29e5b Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Wed, 25 Oct 2023 18:51:41 +0200 Subject: [PATCH 2/7] packages: fix up parse_accept_language() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make several fixes to our handling of the `Accept-Language:` header: - we used to crash if `q=` contained invalid values due to an unchecked cast with `float()`. Catch that error and ignore the entry in that case. - our handling of English was incorrect. We would look for and fail to find po.en.js, and move on to the next item in the list. That means if the user listed English first, followed by another language which we did support, they'd see Cockpit in that other language, which is unexpected. This is a regression introduced by f4be9066c9. Now we drop all items that sort after English. - our handling of fallbacks (ie: 'de' from 'de-de') was incorrect. RFC4647 §3.4 says that a "Lookup" should consider items in the order they're found, stripping each item down to its base form, before considering the next item. This passes a gut check, as well: a user who lists `de-de, nl` probably expects to see a German translation before a Dutch one. We also now mark the parser code as `@lru_cache`. This makes sense: within a given run of cockpit-bridge, we're likely to see very many copies of the Accept-Language header, and they're practically always going to be identical. Make sure we now accept and return immutable types to prevent weird side-effects. We also take the time to remove duplicate items from the list. While we're at it, up our testing game a bit to make sure we don't mistreat requests for 'English' in the future. Closes #19526 Cherry-picked from b6564b2e45481e9eb as a prerequisite for cleanly cheryy-picking important fixes. --- src/cockpit/packages.py | 74 +++++++++++++++++++++++------------- test/pytest/test_packages.py | 63 +++++++++++++++++++++++++++--- 2 files changed, 104 insertions(+), 33 deletions(-) diff --git a/src/cockpit/packages.py b/src/cockpit/packages.py index 938b6203fc2..897540167a9 100644 --- a/src/cockpit/packages.py +++ b/src/cockpit/packages.py @@ -62,28 +62,55 @@ logger = logging.getLogger(__name__) -def parse_accept_language(headers: JsonObject) -> List[str]: +# In practice, this is going to get called over and over again with exactly the +# same list. Let's try to cache the result. +@functools.lru_cache() +def parse_accept_language(accept_language: str) -> Sequence[str]: """Parse the Accept-Language header, if it exists. - Returns an ordered list of languages. + Returns an ordered list of languages, with fallbacks inserted, and + truncated to the position where 'en' would have otherwise appeared, if + applicable. https://tools.ietf.org/html/rfc7231#section-5.3.5 + https://datatracker.ietf.org/doc/html/rfc4647#section-3.4 """ - locales = [] - for language in get_str(headers, 'Accept-Language', '').split(','): - language = language.strip() - locale, _, weightstr = language.partition(';q=') - weight = float(weightstr or 1) - - # Skip possible empty locales - if not locale: - continue - - # Locales are case-insensitive and we store our list in lowercase - locales.append((locale.lower(), weight)) - - return [locale for locale, _ in sorted(locales, key=lambda k: k[1], reverse=True)] + logger.debug('parse_accept_language(%r)', accept_language) + locales_with_q = [] + for entry in accept_language.split(','): + entry = entry.strip().lower() + logger.debug(' entry %r', entry) + locale, _, qstr = entry.partition(';q=') + try: + q = float(qstr or 1.0) + except ValueError: + continue # ignore malformed entry + + while locale: + logger.debug(' adding %r q=%r', locale, q) + locales_with_q.append((locale, q)) + # strip off '-detail' suffixes until there's nothing left + locale, _, _region = locale.rpartition('-') + + # Sort the list by highest q value. Otherwise, this is a stable sort. + locales_with_q.sort(key=lambda pair: pair[1], reverse=True) + logger.debug(' sorted list is %r', locales_with_q) + + # If we have 'en' anywhere in our list, ignore it and all items after it. + # This will result in us getting an untranslated (ie: English) version if + # none of the more-preferred languages are found, which is what we want. + # We also take the chance to drop duplicate items. Note: both of these + # things need to happen after sorting. + results = [] + for locale, _q in locales_with_q: + if locale == 'en': + break + if locale not in results: + results.append(locale) + + logger.debug(' results list is %r', results) + return tuple(results) def sortify_version(version: str) -> str: @@ -243,22 +270,15 @@ def load_file(self, filename: str) -> Document: return Document(path.open('rb'), content_type, content_encoding, content_security_policy) - def load_translation(self, path: str, locales: List[str]) -> Document: + def load_translation(self, path: str, locales: Sequence[str]) -> Document: self.ensure_scanned() assert self.translations is not None - # First check the locales that the user sent + # First match wins for locale in locales: with contextlib.suppress(KeyError): return self.load_file(self.translations[path][locale]) - # Next, check the language-only versions of variant-specified locales - for locale in locales: - language, _, region = locale.partition('-') - if region: - with contextlib.suppress(KeyError): - return self.load_file(self.translations[path][language]) - # We prefer to return an empty document than 404 in order to avoid # errors in the console when a translation can't be found return Document(io.BytesIO(), 'text/javascript') @@ -269,7 +289,7 @@ def load_path(self, path: str, headers: JsonObject) -> Document: assert self.translations is not None if path in self.translations: - locales = parse_accept_language(headers) + locales = parse_accept_language(get_str(headers, 'Accept-Language', '')) return self.load_translation(path, locales) else: return self.load_file(self.files[path]) @@ -499,7 +519,7 @@ def load_manifests_js(self, headers: JsonObject) -> Document: chunks: List[bytes] = [] # Send the translations required for the manifest files, from each package - locales = parse_accept_language(headers) + locales = parse_accept_language(get_str(headers, 'Accept-Language', '')) for name, package in self.packages.items(): if name in ['static', 'base1']: continue diff --git a/test/pytest/test_packages.py b/test/pytest/test_packages.py index 8f3a8f1cd64..e6d7c75c7f6 100644 --- a/test/pytest/test_packages.py +++ b/test/pytest/test_packages.py @@ -16,7 +16,6 @@ # along with this program. If not, see . import json -from typing import List import pytest @@ -24,12 +23,26 @@ @pytest.mark.parametrize(("test_input", "expected"), [ - ('de-at, zh-CH, en,', ['de-at', 'zh-ch', 'en']), - ('es-es, nl;q=0.8, fr;q=0.9', ['es-es', 'fr', 'nl']), - ('fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5', ['fr-ch', 'fr', 'en', 'de', '*']) + # correct handles empty values + ('', ()), + (' ', ()), + (' , ', ()), + (' , ,xx', ('xx',)), + # english → empty list + ('en', ()), + (' , en', ()), + # invalid q values get ignored + ('aa;q===,bb;q=abc,cc;q=.,zz', ('zz',)), + # variant-peeling works + ('aa-bb-cc-dd,ee-ff-gg-hh', ('aa-bb-cc-dd', 'aa-bb-cc', 'aa-bb', 'aa', 'ee-ff-gg-hh', 'ee-ff-gg', 'ee-ff', 'ee')), + # sorting and english-truncation are working + ('fr-ch;q=0.8,es-mx;q=1.0,en-ca;q=0.9', ('es-mx', 'es', 'en-ca')), + ('de-at, zh-CN, en,', ('de-at', 'de', 'zh-cn', 'zh')), + ('es-es, nl;q=0.8, fr;q=0.9', ('es-es', 'es', 'fr', 'nl')), + ('fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5', ('fr-ch', 'fr')) ]) -def test_parse_accept_language(test_input: str, expected: List[str]) -> None: - assert parse_accept_language({'Accept-Language': test_input}) == expected +def test_parse_accept_language(test_input: str, expected: 'tuple[str]') -> None: + assert parse_accept_language(test_input) == expected @pytest.fixture @@ -160,6 +173,44 @@ def test_condition_hides_priority(pkgdir): assert packages.packages['basic'].priority == 1 +def test_english_translation(pkgdir): + make_package(pkgdir, 'one') + (pkgdir / 'one' / 'po.de.js').write_text('eins') + + packages = Packages() + + # make sure we get German + document = packages.load_path('/one/po.js', {'Accept-Language': 'de'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'eins' + + # make sure we get German here (higher q-value) even with English first + document = packages.load_path('/one/po.js', {'Accept-Language': 'en;q=0.9, de-ch'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'eins' + + # make sure we get the empty ("English") translation, and not German + document = packages.load_path('/one/po.js', {'Accept-Language': 'en, de'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + document = packages.load_path('/one/po.js', {'Accept-Language': 'de;q=0.9, fr;q=0.7, en'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + document = packages.load_path('/one/po.js', {'Accept-Language': 'de;q=0.9, fr, en-ca'}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + document = packages.load_path('/one/po.js', {'Accept-Language': ''}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + document = packages.load_path('/one/po.js', {}) + assert '/javascript' in document.content_type + assert document.data.read() == b'' + + def test_translation(pkgdir): # old style: make sure po.de.js is served as fallback for manifest translations make_package(pkgdir, 'one') From a61ccc1474f8b1c6a4ee8eaf447ef3ac056f24d8 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 8 Dec 2023 10:02:30 +0100 Subject: [PATCH 3/7] bridge: Add back menu/tool names to `cockpit-bridge --packages` https://issues.redhat.com/browse/RHEL-19004 Cherry-picked from 7656e7bb459af20743 --- src/cockpit/packages.py | 10 ++++++++-- test/verify/check-connection | 3 ++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/cockpit/packages.py b/src/cockpit/packages.py index 897540167a9..6cbc3a4b982 100644 --- a/src/cockpit/packages.py +++ b/src/cockpit/packages.py @@ -20,6 +20,7 @@ import functools import gzip import io +import itertools import json import logging import mimetypes @@ -489,8 +490,13 @@ def load(self) -> None: def show(self): for name in sorted(self.packages): package = self.packages[name] - menuitems = '' - print(f'{name:20} {menuitems:40} {package.path}') + menuitems = [] + for entry in itertools.chain( + package.manifest.get('menu', {}).values(), + package.manifest.get('tools', {}).values()): + with contextlib.suppress(KeyError): + menuitems.append(entry['label']) + print(f'{name:20} {", ".join(menuitems):40} {package.path}') def get_bridge_configs(self) -> Sequence[BridgeConfig]: def yield_configs(): diff --git a/test/verify/check-connection b/test/verify/check-connection index af7bbc30a86..160633dee0c 100755 --- a/test/verify/check-connection +++ b/test/verify/check-connection @@ -1200,7 +1200,8 @@ UnixPath=/run/cockpit/session packages = m.execute("cockpit-bridge --packages") self.assertRegex(packages, r"(^|\n)base1\s+.*/usr/share/cockpit/base1") - self.assertRegex(packages, r"(^|\n)system\s+.*/usr/share/cockpit/systemd") + # also includes menu and tools entries + self.assertRegex(packages, r"(^|\n)system\s.*Services.*Terminal.*\s/usr/share/cockpit/systemd") @testlib.skipDistroPackage() From 5c512893a3f106285b7dd437815372347e95e124 Mon Sep 17 00:00:00 2001 From: Allison Karlitskaya Date: Fri, 8 Dec 2023 09:52:35 +0100 Subject: [PATCH 4/7] bridge: Add back support for x.min.js files We removed support for `.min.` filenames because we thought that nobody was using it anymore, but there are still external packages that make use of it. Add it back again, but only for the case where the non-minified version isn't present. The minified version can always be explicitly requested via its full filename (minus `.gz`, if applicable). Add a bit of debugging output and some test cases to make sure we handle the combinations properly. Fixes https://issues.redhat.com/browse/RHEL-19005 Cherry-picked from 5d00faa6bb52153341d71 --- src/cockpit/packages.py | 13 +++++++++- test/pytest/test_packages.py | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/cockpit/packages.py b/src/cockpit/packages.py index 6cbc3a4b982..5d882ef6742 100644 --- a/src/cockpit/packages.py +++ b/src/cockpit/packages.py @@ -229,11 +229,22 @@ def ensure_scanned(self) -> None: # Accept-Language is case-insensitive and uses '-' to separate variants lower_locale = locale.lower().replace('_', '-') + logger.debug('Adding translation %r %r -> %r', basename, lower_locale, name) self.translations[f'{basename}.js'][lower_locale] = name else: - basename = name[:-3] if name.endswith('.gz') else name + # strip out trailing '.gz' components + basename = re.sub('.gz$', '', name) + logger.debug('Adding content %r -> %r', basename, name) self.files[basename] = name + # If we see a filename like `x.min.js` we want to also offer it + # at `x.js`, but only if `x.js(.gz)` itself is not present. + # Note: this works for both the case where we found the `x.js` + # first (it's already in the map) and also if we find it second + # (it will be replaced in the map by the line just above). + # See https://github.com/cockpit-project/cockpit/pull/19716 + self.files.setdefault(basename.replace('.min.', '.'), name) + # support old cockpit-po-plugin which didn't write po.manifest.??.js if not self.translations['po.manifest.js']: self.translations['po.manifest.js'] = self.translations['po.js'] diff --git a/test/pytest/test_packages.py b/test/pytest/test_packages.py index e6d7c75c7f6..4170f5d50b7 100644 --- a/test/pytest/test_packages.py +++ b/test/pytest/test_packages.py @@ -239,3 +239,49 @@ def test_translation(pkgdir): assert b'eins\n' in contents assert b'zwo\n' in contents assert b'zwei\n' not in contents + + +def test_filename_mangling(pkgdir): + make_package(pkgdir, 'one') + + # test various filename variations + (pkgdir / 'one' / 'one.js').write_text('this is one.js') + (pkgdir / 'one' / 'two.js.gz').write_text('this is two.js') + (pkgdir / 'one' / 'three.min.js.gz').write_text('this is three.js') + (pkgdir / 'one' / 'four.min.js').write_text('this is four.js') + + packages = Packages() + encodings = set() + + for name in ['one', 'two', 'three', 'four']: + document = packages.load_path(f'/one/{name}.js', {}) + assert document.data.read().decode() == f'this is {name}.js' + assert '/javascript' in document.content_type + encodings.add(document.content_encoding) + + assert encodings == {None, 'gzip'} # make sure we saw both compressed and uncompressed + + +def test_overlapping_minified(pkgdir): + make_package(pkgdir, 'one') + (pkgdir / 'one' / 'one.min.js').write_text('min') + (pkgdir / 'one' / 'one.js').write_text('max') + + # try the other way around in hope of listing the files in reverse order + (pkgdir / 'one' / 'two.js').write_text('max') + (pkgdir / 'one' / 'two.min.js').write_text('min') + + packages = Packages() + + # if both files are present, we should find the original one + document = packages.load_path('/one/one.js', {}) + assert document.data.read().decode() == 'max' + document = packages.load_path('/one/two.js', {}) + assert document.data.read().decode() == 'max' + + # but requesting .min. explicitly will load it + document = packages.load_path('/one/one.min.js', {}) + assert document.data.read().decode() == 'min' + document = packages.load_path('/one/two.min.js', {}) + assert document.data.read().decode() == 'min' + From 9d10974a30be9eef6f4c1c21f4efb13caa4b903f Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Fri, 8 Dec 2023 09:26:32 +0100 Subject: [PATCH 5/7] client: Provide fallback for GLib.get_user_state_dir() This call was introduced in commit 483027a55, but this API only exists since glib 2.72, while current RHEL 9.3 still has 2.68. This causes client to crash with an AttributeError. Provide a fallback implementation. https://issues.redhat.com/browse/RHEL-18989 Cherry-picked from 82c59a01e01dba1 --- src/client/cockpit-client | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/client/cockpit-client b/src/client/cockpit-client index 68eb110b317..003ded3de0d 100755 --- a/src/client/cockpit-client +++ b/src/client/cockpit-client @@ -55,6 +55,17 @@ def prctl(*args): raise Exception('prctl() failed') +def get_user_state_dir(): + try: + # GLib ≥ 2.72 + return GLib.get_user_state_dir() + except AttributeError: + try: + return os.environ["XDG_STATE_HOME"] + except KeyError: + return os.path.expanduser("~/.local/share") + + prctl.SET_PDEATHSIG = 1 @@ -222,7 +233,7 @@ class CockpitClient(Gtk.Application): context.set_sandbox_enabled(enabled=True) context.set_cache_model(WebKit2.CacheModel.DOCUMENT_VIEWER) - cookiesFile = os.path.join(GLib.get_user_state_dir(), "cockpit-client", "cookies.txt") + cookiesFile = os.path.join(get_user_state_dir(), "cockpit-client", "cookies.txt") cookies = context.get_cookie_manager() cookies.set_persistent_storage(cookiesFile, WebKit2.CookiePersistentStorage.TEXT) From eefbdd3a70a2b47e378c7dd8055399a4078240e3 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Tue, 5 Sep 2023 13:44:59 +0200 Subject: [PATCH 6/7] apps: Consider ID_LIKE for mapping AppStream data packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Cherry-picked from 7bbf9168ac330fc3c --- pkg/apps/application-list.jsx | 22 ++++++++++++----- pkg/apps/manifest.json | 4 ++-- test/verify/check-apps | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/pkg/apps/application-list.jsx b/pkg/apps/application-list.jsx index 7afd38483f5..a8ed72aa778 100644 --- a/pkg/apps/application-list.jsx +++ b/pkg/apps/application-list.jsx @@ -98,11 +98,21 @@ export const ApplicationList = ({ metainfo_db, appProgress, appProgressTitle, ac comps.push(metainfo_db.components[id]); comps.sort((a, b) => a.name.localeCompare(b.name)); - 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+/)]; + if (cockpit.manifests.apps && cockpit.manifests.apps.config) { let val = cockpit.manifests.apps.config[name]; - if (typeof val === 'object' && val !== null && !Array.isArray(val)) - val = val[distro_id]; + if (typeof val === 'object' && val !== null && !Array.isArray(val)) { + os_list.find(c => { + if (val[c]) { + val = val[c]; + return true; + } + return false; + }); + } return val !== undefined ? val : def; } else { return def; @@ -112,8 +122,8 @@ export const ApplicationList = ({ metainfo_db, appProgress, appProgressTitle, ac function refresh() { read_os_release().then(os_release => PackageKit.refresh(metainfo_db.origin_files, - get_config('appstream_config_packages', os_release.ID, []), - get_config('appstream_data_packages', os_release.ID, []), + get_config('appstream_config_packages', os_release, []), + get_config('appstream_data_packages', os_release, []), setProgress)) .finally(() => setProgress(false)) .catch(show_error); @@ -121,7 +131,7 @@ export const ApplicationList = ({ metainfo_db, appProgress, appProgressTitle, ac let refresh_progress, refresh_button, tbody; if (progress) { - refresh_progress = ; + refresh_progress = ; refresh_button = ; } else { refresh_progress = null; diff --git a/pkg/apps/manifest.json b/pkg/apps/manifest.json index 0679c1b3bf2..56039e57f53 100644 --- a/pkg/apps/manifest.json +++ b/pkg/apps/manifest.json @@ -14,10 +14,10 @@ "config": { "appstream_config_packages": { - "debian": ["appstream"], "ubuntu": ["appstream"] + "debian": ["appstream"] }, "appstream_data_packages": { - "fedora": ["appstream-data"], "rhel": ["appstream-data"] + "fedora": ["appstream-data"] } } } diff --git a/test/verify/check-apps b/test/verify/check-apps index ac4cfd7b7fe..bd6d1396733 100755 --- a/test/verify/check-apps +++ b/test/verify/check-apps @@ -16,6 +16,7 @@ # # You should have received a copy of the GNU Lesser General Public License # along with Cockpit; If not, see . +import time import packagelib import testlib @@ -129,6 +130,50 @@ class TestApps(packagelib.PackageCase): def testWithUrlRoot(self): self.testBasic(urlroot="/webcon") + def testOsMap(self): + b = self.browser + m = self.machine + + self.allow_journal_messages("can't remove watch: Invalid argument") + + self.restore_dir("/usr/share/metainfo", reboot_safe=True) + self.restore_dir("/usr/share/app-info", reboot_safe=True) + self.restore_dir("/var/cache/app-info", reboot_safe=True) + + # Make sure none of the appstream directories exist. They + # will be created later and we need to cope with that. + m.execute("rm -rf /usr/share/metainfo /usr/share/app-info /var/cache/app-info") + + # use a fake distro map + self.write_file("/etc/cockpit/apps.override.json", + '{ "config": { "appstream_data_packages":' + ' {"testy": ["appstream-data-test"], "otheros": ["nosuchpackage"]}' + ' }}') + + self.createAppStreamPackage("app-1", "1.0", "1") + self.createAppStreamRepoPackage() + + self.login_and_go("/apps") + b.wait_visible(".pf-v5-c-empty-state") + + # os-release is a symlink target, don't clobber that + self.restore_file("/etc/os-release") + m.execute("rm /etc/os-release") + + # unknown OS: nothing gets installed + m.write("/etc/os-release", 'ID="unmapped"\nID_LIKE="mysterious"\nVERSION_ID="1"\n') + b.click("#refresh") + # the progress bar is too fast to reliably catch it + time.sleep(1) + b.wait_not_present("#refresh-progress") + b.wait_visible(".pf-v5-c-empty-state") + + # known OS: appstream-data-tst gets installed from the map + m.write("/etc/os-release", 'ID="derivative"\nID_LIKE="spicy testy"\nVERSION_ID="1"\n') + b.click("#refresh") + with b.wait_timeout(30): + b.wait_visible(".app-list #app-1") + def testL10N(self): b = self.browser m = self.machine From cfecdb8b70ba3ddb6cf3605df4c7ca14642ea65a Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Wed, 13 Sep 2023 12:47:25 +0200 Subject: [PATCH 7/7] apps: Fix reading AppStream metadata config Commit 7bbf9168ac33 introduced a major bug: If the OS in os-release was not found in the iteration, get_config() would return the whole map instead of the default value. That crashed the page with `TypeError: names.forEach is not a function`. This is awkward to do correctly with .find(), especially due to modifying `val` as a side effect; iterate over `os_list` the good old `for` way instead. This was hidden by the test's manifest override leaving `appstream_config_packages` intact, so it was always taken from the real underlying OS. Override it as well. Cherry-picked from 0eeb475de491ab --- pkg/apps/application-list.jsx | 14 ++++++-------- test/verify/check-apps | 3 ++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/apps/application-list.jsx b/pkg/apps/application-list.jsx index a8ed72aa778..438ea9fc9cd 100644 --- a/pkg/apps/application-list.jsx +++ b/pkg/apps/application-list.jsx @@ -103,15 +103,13 @@ export const ApplicationList = ({ metainfo_db, appProgress, appProgressTitle, ac const os_list = [os_release.ID || "", ...(os_release.ID_LIKE || "").split(/\s+/)]; if (cockpit.manifests.apps && cockpit.manifests.apps.config) { - let val = cockpit.manifests.apps.config[name]; + const val = cockpit.manifests.apps.config[name]; if (typeof val === 'object' && val !== null && !Array.isArray(val)) { - os_list.find(c => { - if (val[c]) { - val = val[c]; - return true; - } - return false; - }); + for (const os of os_list) { + if (val[os]) + return val[os]; + } + return def; } return val !== undefined ? val : def; } else { diff --git a/test/verify/check-apps b/test/verify/check-apps index bd6d1396733..b57de7c58ac 100755 --- a/test/verify/check-apps +++ b/test/verify/check-apps @@ -147,7 +147,8 @@ class TestApps(packagelib.PackageCase): # use a fake distro map self.write_file("/etc/cockpit/apps.override.json", '{ "config": { "appstream_data_packages":' - ' {"testy": ["appstream-data-test"], "otheros": ["nosuchpackage"]}' + ' {"testy": ["appstream-data-test"], "otheros": ["nosuchpackage"]},' + ' "appstream_config_packages": []' ' }}') self.createAppStreamPackage("app-1", "1.0", "1")