PR#160: Fix insertion of comments, parsing of os-release, and bump status to "production" - fedora-rust/rust2rpm

fedora-rust / rust2rpm

#160 Fix insertion of comments, parsing of os-release, and bump status to "production"

Merged 2 years ago by zbyszek. Opened 2 years ago by zbyszek.

fedora-rust/ zbyszek/rust2rpm os-release-parsing into main

setup: bump development status to "production"

Zbigniew Jędrzejewski-Szmek • 2 years ago

df09a9d

Import the os-release parser from the man page

Zbigniew Jędrzejewski-Szmek • 2 years ago

02a76c3

Fix insertion of newlines in license conversion comments

Zbigniew Jędrzejewski-Szmek • 2 years ago

1100c60

rust2rpm/__main__.py

file modified

+22 -14

		`@@ -1,4 +1,5 @@`
		`import argparse`
		`+ import ast`
		`import configparser`
		`import contextlib`
		`from datetime import datetime, timezone`
		`@@ -52,26 +53,33 @@`
		`return sorted(func(args, *kwargs))`
		`return functools.update_wrapper(wrapper, func)`

		`- def get_default_target():`
		`+ def read_os_release():`
		`try:`
		`- os_release_file = open('/etc/os-release')`
		`+ f = open('/etc/os-release')`
		`except FileNotFoundError:`
		`- os_release_file = open('/usr/lib/os-release')`
		`- with os_release_file:`
		`- conf = configparser.ConfigParser()`
		`- conf.read_file(itertools.chain(["[os-release]"], os_release_file))`
		`- os_release = conf["os-release"]`
		`+ f = open('/usr/lib/os-release')`
		`+`
		`+ for line in f:`
		`+ line = line.rstrip()`
ignatenkobrain commented 2 years ago you can't have spaces at the beginning? :(
		`+ if not line or line.startswith('#'):`
		`+ continue`
		`+ m = re.match(r'([A-Z][A-Z_0-9]+)=(.*)', line)`
		`+ if m:`
ignatenkobrain commented 2 years ago So we want to ignore invalid lines completely?
		`+ name, val = m.groups()`
		`+ if val and val[0] in '"\'':`
ignatenkobrain commented 2 years ago should not this be: `… in {'"', "'"}` to avoid iterating over the string?
		`+ val = ast.literal_eval(val)`
		`+ yield name, val`
		`+`
		`+ def get_default_target():`
		`+ os_release = dict(read_os_release())`
		`os_id = os_release.get("ID")`
		`- os_like = os_release.get("ID_LIKE")`
		`- if os_like is not None:`
		`- os_like = shlex.split(os_like)`
		`- else:`
		`- os_like = []`
		`+ # ID_LIKE is a space-separated list of identifiers like ID`
		`+ os_like = os_release.get("ID_LIKE", "").split()`

		`# Order matters here!`
		`- if os_id == "mageia" or ("mageia" in os_like):`
		`+ if "mageia" in (os_id, *os_like):`
		`return "mageia"`
		`- elif os_id == "fedora" or ("fedora" in os_like):`
		`+ elif "fedora" in (os_id, *os_like):`
		`return "fedora"`
		`elif "suse" in os_like:`
		`return "opensuse"`

rust2rpm/licensing.py

file modified

+5 -5

		`@@ -26,7 +26,7 @@`
		`print(f"{k} → {v}", file=file)`

		`def translate_license_fedora(license):`
		`- comments = ''`
		`+ comments = []`
		`final = []`
		`for tag in license.split():`
		`# We accept all variant cases, but output lowercase which is what Fedora LicensingGuidelines specify`
		`@@ -43,18 +43,18 @@`

		`mapped = spdx_to_fedora_map().get(key, None)`
		`if mapped is None:`
		`- comments += f'# FIXME: Upstream uses unknown SPDX tag {fulltag}!'`
		`+ comments += [f'# FIXME: Upstream uses unknown SPDX tag {fulltag}!']`
		`final.append(tag)`
		`elif mapped == '':`
		`- comments += f"# FIXME: Upstream SPDX tag {fulltag} not listed in Fedora's good licenses list.\n"`
		`- comments += "# FIXME: This package might not be allowed in Fedora!\n"`
		`+ comments += [f"# FIXME: Upstream SPDX tag {fulltag} not listed in Fedora's good licenses list.",`
		`+ "# FIXME: This package might not be allowed in Fedora!"]`
		`final.append(tag)`
		`else:`
		`final.append(mapped)`
		`if mapped != tag:`
		`print(f'Upstream license tag {fulltag} translated to {mapped}',`
		`file=_sys.stderr)`
		`- return (' '.join(final), comments or None)`
		`+ return (' '.join(final), '\n'.join(comments) or None)`

		`def translate_license(target, license):`
		`license = translate_slashes(license)`

setup.py

file modified

+1 -1

		`@@ -41,7 +41,7 @@`
		`author_email="ignatenkobrain@fedoraproject.org",`
		`url="https://pagure.io/fedora-rust/rust2rpm",`
		`classifiers=[`
		`- "Development Status :: 4 - Beta",`
		`+ "Development Status :: 5 - Production/Stable",`
		`"Intended Audience :: Developers",`
		`"License :: OSI Approved :: MIT License",`
		`"Operating System :: POSIX :: Linux",`

no initial comment

ignatenkobrain commented on line 25 of rust2rpm/__main__.py 2 years ago

you can't have spaces at the beginning? :(

ignatenkobrain commented on line 29 of rust2rpm/__main__.py 2 years ago

So we want to ignore invalid lines completely?

ignatenkobrain commented on line 31 of rust2rpm/__main__.py 2 years ago

should not this be: … in {'"', "'"} to avoid iterating over the string?

zbyszek commented 2 years ago

you can't have spaces at the beginning? :(

The man page says no.

So we want to ignore invalid lines completely?

Yeah. I think that's reasonable here: this is not supposed to verify the os-release file, just make use of it. The example in the man page warns about unparseable lines, but I removed this here. And I think this doesn't matter in practice: the distros have well-formed os-release files, it's not something you'd usually edit by hand.

should not this be: … in {'"', "'"} to avoid iterating over the string?

Well, either we iterate over the (two character) string, or we iterate over the two-element set with each of the elements being one character. I don't think it matters either way, but the latter involves more objects and is not going to be faster.

ignatenkobrain commented 2 years ago

Well, either we iterate over the (two character) string, or we iterate over the two-element set with each of the elements being one character. I don't think it matters either way, but the latter involves more objects and is not going to be faster.

The set should be faster I think. Not that I care about performance here but just curious :)

ignatenkobrain commented 2 years ago

In [5]: %timeit "'" in {'"', "'"}                                                                                                                                                                                                             
20.3 ns ± 0.202 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [6]: %timeit "'" in '"\''                                                                                                                                                                                                                  
21.8 ns ± 0.23 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [7]: %timeit '"' in '"\''                                                                                                                                                                                                                  
22.1 ns ± 0.46 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [8]: %timeit '"' in {'"', "'"}                                                                                                                                                                                                             
20.4 ns ± 0.272 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

zbyszek commented 2 years ago

Commit d93c499 fixes this pull-request