The ebuild has multiple QA violations, indicating that the author neither read the docs (esp. Python Guide), consulted existing good quality ebuilds, nor run `pkgcheck check --commit`. > # Copyright 1999-2023 Gentoo Authors Not updated. > LICENSE="GPL-2" All files with copyright header have a GPL-2+ header. > # testing requires local network (e.g. spin up slapd, httpd) > #PROPERTIES="test_network" > #RESTRICT="test" What does this comment mean? Are you saying that the tests that you've enabled below don't work, or is it obsolete? > # Testing: > # *unit* tests do not require networking. > # *integration* tests require openldap's slapd and networking Again, this comment doesn't match what the ebuild does. > dev-python/pytest-cov[${PYTHON_USEDEP}] We don't run coverage testing in Gentoo. You are supposed to strip these flags, as the docs say. > distutils-r1_python_compile --verbose What does exactly the `--verbose` do here? Yes, it is a trick question because I'm pretty sure it does nothing. > # Do not install the tests as functional source. > find "${D}" \ > -path '*/site-packages/nss_cache/*' \( \ > -iname '*_test.py*' \ > -o -iname '*_test.*.py*' \ > \) \ > -delete Removing tests that do not collide with other packages is utter nonsense but *if you insist*, then you are supposed to prevent them from being installed instead of engaging in horribly complex logic to strip files and their caches afterwards. Also '|| die'. > # Ignore any exit code from find. > return 0 You are not supposed to ignore exit codes. If find exits unsuccessfully, then it means that the invocation is broken. > distutils_enable_tests pytest We put these things between metadata and phase functions, where people can actually see them, instead of at the very bottom.
(In reply to Michał Górny from comment #0) > The ebuild has multiple QA violations, indicating that the author neither > read the docs (esp. Python Guide), consulted existing good quality ebuilds, > nor run `pkgcheck check --commit`. robbat2@bohr-int:/usr/portage/net-nds/nsscache $ pkgcheck scan net-nds/nsscache UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ 0.49 ] There are no reported QA violations from that pkgcheck output. > > > # Copyright 1999-2023 Gentoo Authors > Not updated. Fine; will update that. > > LICENSE="GPL-2" > All files with copyright header have a GPL-2+ header. Not changed from previous version. Reflects other declarations in the source: nsscache.spec:License: GPLv2 setup.py: license="GPL", setup.py: "License :: OSI Approved :: GPL", > > > # testing requires local network (e.g. spin up slapd, httpd) > > #PROPERTIES="test_network" > > #RESTRICT="test" > What does this comment mean? Are you saying that the tests that you've > enabled below don't work, or is it obsolete? > > > # Testing: > > # *unit* tests do not require networking. > > # *integration* tests require openldap's slapd and networking > Again, this comment doesn't match what the ebuild does. Obsolete comment. I've verified that the unit tests pass in Portage, and upstream no longer runs the integration tests outside of Docker. Do you really want me to run full integration testing in Portage? > > > dev-python/pytest-cov[${PYTHON_USEDEP}] > > We don't run coverage testing in Gentoo. You are supposed to strip these > flags, as the docs say. Fixed. > > > distutils-r1_python_compile --verbose > > What does exactly the `--verbose` do here? Yes, it is a trick question > because I'm pretty sure it does nothing. distutils-r1_python_compile passes it down to esetup.py; and onwards down to setup.py; explicitly overrides anybody else passing --quiet (and --verbose wasn't always the default of setup.py). P.S. distutils-r1.eclass: nonfatal esetup.py test --verbose > > > # Do not install the tests as functional source. > > find "${D}" \ > > -path '*/site-packages/nss_cache/*' \( \ > > -iname '*_test.py*' \ > > -o -iname '*_test.*.py*' \ > > \) \ > > -delete > Removing tests that do not collide with other packages is utter nonsense but > *if you insist*, then you are supposed to prevent them from being installed > instead of engaging in horribly complex logic to strip files and their > caches afterwards. Also '|| die'. I wanted to take this particular point up with Python team in general. Why are the test files being installed to the final system? Please make installing them a QA issue, and make them go away everywhere. Recently on the LiveGUI media: /usr/lib/python*/ctypes/test /usr/lib/python*/distutils/tests /usr/lib/python*/lib2to3/tests /usr/lib/python*/site-packages/Cython/**/Tests/ (and many more) This would be a 100MB+ pre-squashfs saving for the LiveGUI media. As to the how of removing them; setup.py install seems to always install them regardless of how I tried: """ --- setup.py +++ setup.py @@ -63,4 +63,15 @@ as LDAP.""", "consul": ["pycurl"], "gcs": ["google-cloud-storage"], }, + include_package_data=True, + exclude_package_data={ + '': ['*_test.py'], + '*': ['*_test.py'], + "nss_cache": ['*_test.py'], + "nss_cache.caches": ['*_test.py'], + "nss_cache.maps": ['*_test.py'], + "nss_cache.util": ['*_test.py'], + "nss_cache.update": ['*_test.py'], + "nss_cache.sources": ['*_test.py'], + } ) """ If there's a "official" way to NOT install them, that doesn't require deleting them on disk in $WORKDIR, I'll happily switch to it. > > # Ignore any exit code from find. > > return 0 > > You are not supposed to ignore exit codes. If find exits unsuccessfully, > then it means that the invocation is broken. Removed. > > distutils_enable_tests pytest > > We put these things between metadata and phase functions, where people can > actually see them, instead of at the very bottom. If that's what you want, put it in your docs: https://projects.gentoo.org/python/guide/test.html#tests-in-python-packages Says nothing about the style of where it must be. Lastly here, can we please have a discussion about your approach: As somebody who does a reasonable job following all of the email on the gentoo-dev mailing list, and many other lists, and your implication of "you must follow the entire Gentoo Python Guide if you use the distutils eclasses in any ebuild in the entire tree" feels like a large communications miss on the part of the Python team, as well as an overly abrasive approach, veering close on being personal attacks. Date: 4 Mar 2020 15:32:15 +0100 From: Michał Górny <mgorny@gentoo.org> Subject: [gentoo-dev] [PATCH 1/9] python*-r1.eclass, distutils-r1.eclass: Link to Python Guide ... +# For more information, please see the Python Guide: +# https://dev.gentoo.org/~mgorny/python-guide/ Nothing about that says it's best practice, or mandatory. Just for more informatation. My *personal* QA checking of ebuilds: - pkgcheck scan => ensure no *new* errors, try to fix legacy errors - QA output from installing the ebuilds, running tests => fix those - is the result of the ebuild correct? Are the ebuilds "perfect"? No. It the ebuild good enough? Yes If something is slightly broken, or less than optimal, it can be fixed. If the fault is something that's so dire, it should have automated QA checking. It's not about getting the ebuild perfect every time. If you want to make these rules mandatory: 1. Implement easy-to-run automated checking for each of the rules (and make it the default in pkgcheck). 2. Announce it clearly.
(In reply to Robin Johnson from comment #1) > (In reply to Michał Górny from comment #0) > > The ebuild has multiple QA violations, indicating that the author neither > > read the docs (esp. Python Guide), consulted existing good quality ebuilds, > > nor run `pkgcheck check --commit`. > > robbat2@bohr-int:/usr/portage/net-nds/nsscache $ pkgcheck scan > net-nds/nsscache > UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ > 0.49 ] > > There are no reported QA violations from that pkgcheck output. You've omitted the significant `--commit` option. It's too late to reproduce that now. > > > LICENSE="GPL-2" > > All files with copyright header have a GPL-2+ header. > Not changed from previous version. > Reflects other declarations in the source: > nsscache.spec:License: GPLv2 > setup.py: license="GPL", > setup.py: "License :: OSI Approved :: GPL", Are you suggesting that the license should be just "GPL" then? Determining the correct license requires some more effort than copying a string from a random file and ignoring the whole context. > > > # testing requires local network (e.g. spin up slapd, httpd) > > > #PROPERTIES="test_network" > > > #RESTRICT="test" > > What does this comment mean? Are you saying that the tests that you've > > enabled below don't work, or is it obsolete? > > > > > # Testing: > > > # *unit* tests do not require networking. > > > # *integration* tests require openldap's slapd and networking > > Again, this comment doesn't match what the ebuild does. > Obsolete comment. I've verified that the unit tests pass in Portage, and > upstream no longer runs the integration tests outside of Docker. > Do you really want me to run full integration testing in Portage? I don't have an opinion on what. What I expect is for the comments to clearly communicate to future maintainer what the ebuild does, or at least don't introduce unnecessary confusion, compared to no comment at all. The last thing we need is people playing archeology, trying to figure out what the previous maintainer meant. > > > distutils-r1_python_compile --verbose > > > > What does exactly the `--verbose` do here? Yes, it is a trick question > > because I'm pretty sure it does nothing. > distutils-r1_python_compile passes it down to esetup.py; and onwards down to > setup.py; explicitly overrides anybody else passing --quiet (and --verbose > wasn't always the default of setup.py). No, it doesn't. Not in PEP517 mode. > > > # Do not install the tests as functional source. > > > find "${D}" \ > > > -path '*/site-packages/nss_cache/*' \( \ > > > -iname '*_test.py*' \ > > > -o -iname '*_test.*.py*' \ > > > \) \ > > > -delete > > Removing tests that do not collide with other packages is utter nonsense but > > *if you insist*, then you are supposed to prevent them from being installed > > instead of engaging in horribly complex logic to strip files and their > > caches afterwards. Also '|| die'. > I wanted to take this particular point up with Python team in general. > Why are the test files being installed to the final system? Please make > installing them a QA issue, and make them go away everywhere. If you are concerned about packages installing test files, discuss the problem upstream. Gentoo should follow upstream whenever possible, and I see no reason to change that because you don't like tests. Making tests "go away everywhere" actually breaks stuff. There are packages that import modules from CPython's test suite, for their own tests. Finally, this is Python. Many upstreams actually support running tests on top of the installed package. Gentoo users can do that e.g. to verify that a dependency upgrade didn't break the package. > As to the how of removing them; setup.py install seems to always install > them regardless of how I tried: > """ > --- setup.py > +++ setup.py > @@ -63,4 +63,15 @@ as LDAP.""", > "consul": ["pycurl"], > "gcs": ["google-cloud-storage"], > }, > + include_package_data=True, > + exclude_package_data={ > + '': ['*_test.py'], > + '*': ['*_test.py'], > + "nss_cache": ['*_test.py'], > + "nss_cache.caches": ['*_test.py'], > + "nss_cache.maps": ['*_test.py'], > + "nss_cache.util": ['*_test.py'], > + "nss_cache.update": ['*_test.py'], > + "nss_cache.sources": ['*_test.py'], > + } > ) > > """ > > If there's a "official" way to NOT install them, that doesn't require > deleting them on disk in $WORKDIR, I'll happily switch to it. I don't know. You've invented the problem, and now you expect me to spend a significant effort trying to solve it. > > > distutils_enable_tests pytest > > > > We put these things between metadata and phase functions, where people can > > actually see them, instead of at the very bottom. > If that's what you want, put it in your docs: > https://projects.gentoo.org/python/guide/test.html#tests-in-python-packages > Says nothing about the style of where it must be. All the examples put it in a specific place. > Are the ebuilds "perfect"? No. > It the ebuild good enough? Yes "Good enough" is not sufficient when we're dealing with an ebuild that has been repeatedly subject to negligence and that has been last-rited twice already. When you've already imposed significant work on others, it is only expected that others expect much more than the absolutely minimal effort from you.
(In reply to Michał Górny from comment #2) > (In reply to Robin Johnson from comment #1) > > (In reply to Michał Górny from comment #0) > > > The ebuild has multiple QA violations, indicating that the author neither > > > read the docs (esp. Python Guide), consulted existing good quality ebuilds, > > > nor run `pkgcheck check --commit`. > > > > robbat2@bohr-int:/usr/portage/net-nds/nsscache $ pkgcheck scan > > net-nds/nsscache > > UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ > > 0.49 ] > > > > There are no reported QA violations from that pkgcheck output. > > You've omitted the significant `--commit` option. It's too late to > reproduce that now. Can you please clarify which QA issues it was supposed to detect? And you said did explicitly say "pkgcheck check --commit"; which doesn't seem to exist. ``` # added another commit of QA fixes) $ pkgcheck check --commits pkgcheck: error: argument subcommand: invalid choice: 'check' (choose from 'cache', 'ci', 'replay', 'scan', 'show') $ pkgcheck scan --commits net-nds/nsscache UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ 0.49 ] $ pkgcheck --version pkgcheck 0.10.26 ``` > > > > LICENSE="GPL-2" > > > All files with copyright header have a GPL-2+ header. > > Not changed from previous version. > > Reflects other declarations in the source: > > nsscache.spec:License: GPLv2 > > setup.py: license="GPL", > > setup.py: "License :: OSI Approved :: GPL", > > Are you suggesting that the license should be just "GPL" then? Determining > the correct license requires some more effort than copying a string from a > random file and ignoring the whole context. The LICENSE text was already commented after your first remark. I wasn't the first author of these ebuilds, and it's never come up as any holistic license review that upstreams are inconsistent.``` # upstream *sources* say "or later", but upstream metadata does not include the # 'or later' clause. LICENSE="GPL-2+" ``` Are you really going to tell me that every Gentoo developer checks that every file in every package matches what upstream declares as the license? > > Obsolete comment. I've verified that the unit tests pass in Portage, and > > upstream no longer runs the integration tests outside of Docker. > > Do you really want me to run full integration testing in Portage? > > I don't have an opinion on what. What I expect is for the comments to > clearly communicate to future maintainer what the ebuild does, or at least > don't introduce unnecessary confusion, compared to no comment at all. The > last thing we need is people playing archeology, trying to figure out what > the previous maintainer meant. This was ALREADY clarified in the comments, why are you continuing to complain about it? ``` # Testing: # *unit* tests do not require networking. # *integration* tests require openldap's slapd and networking # # The ebuild runs the unit testing explicitly, as upstream uses Docker to run # the integration tests. ``` > > > > distutils-r1_python_compile --verbose > > > > > > What does exactly the `--verbose` do here? Yes, it is a trick question > > > because I'm pretty sure it does nothing. > > distutils-r1_python_compile passes it down to esetup.py; and onwards down to > > setup.py; explicitly overrides anybody else passing --quiet (and --verbose > > wasn't always the default of setup.py). > No, it doesn't. Not in PEP517 mode. Fine, it's been removed already since the PEP517 mode sufficently the behavior of distutils-r1_python_install, without any clear versioning change. Maybe the eclass should have been distutils-r2 or some clear version change. > > > > > # Do not install the tests as functional source. > > > > find "${D}" \ > > > > -path '*/site-packages/nss_cache/*' \( \ > > > > -iname '*_test.py*' \ > > > > -o -iname '*_test.*.py*' \ > > > > \) \ > > > > -delete > > > Removing tests that do not collide with other packages is utter nonsense but > > > *if you insist*, then you are supposed to prevent them from being installed > > > instead of engaging in horribly complex logic to strip files and their > > > caches afterwards. Also '|| die'. > > I wanted to take this particular point up with Python team in general. > > Why are the test files being installed to the final system? Please make > > installing them a QA issue, and make them go away everywhere. > > If you are concerned about packages installing test files, discuss the > problem upstream. Gentoo should follow upstream whenever possible, and I > see no reason to change that because you don't like tests. > > Making tests "go away everywhere" actually breaks stuff. There are packages > that import modules from CPython's test suite, for their own tests. > > Finally, this is Python. Many upstreams actually support running tests on > top of the installed package. Gentoo users can do that e.g. to verify that > a dependency upgrade didn't break the package. > > > As to the how of removing them; setup.py install seems to always install > > them regardless of how I tried: ... (snip code) > > > > If there's a "official" way to NOT install them, that doesn't require > > deleting them on disk in $WORKDIR, I'll happily switch to it. > > I don't know. You've invented the problem, and now you expect me to spend a > significant effort trying to solve it. Many years ago I recall an instruction that packages should NOT install their tests into ${D}. I think one of the original rationales was that if you were installing the tests, then you should ALSO install all of the USE=test dependencies as RDEPEND. I don't think it's a good idea for them to start doing so, so I'll take it to the -dev mailing list to change this as a distribution (likely with the choice that USE=test should install tests to the filesystem, and be decoupled from FEATURES=test [which means run tests]). > > > > > distutils_enable_tests pytest > > > > > > We put these things between metadata and phase functions, where people can > > > actually see them, instead of at the very bottom. > > If that's what you want, put it in your docs: > > https://projects.gentoo.org/python/guide/test.html#tests-in-python-packages > > Says nothing about the style of where it must be. > > All the examples put it in a specific place. Many ebuilds have distutils_enable_tests as the last lines. But really, putting it in the document would have no lasting impact unless there is automation/tooling to detect and possibly fix that it's not ideal. QA check would be relatively simple line number of "^distutils_enable_tests" must be less than line number of (phase_functions, default to positive infinity), AND greater than line number of (key metadata variables) "We put these things between metadata and phase functions," Here's some dev-python ebuilds where this is not true: dev-python/poetry/poetry-1.7.1.ebuild - very last line, after src_prepare & EPYTEST_DESELECT dev-python/pdoc3/pdoc3-0.10.0-r1.ebuild - very last line, after python_prepare_all I didn't go looking very hard, so there's probably more. And if you wanted to further muddy the waters, the nsscache ebuild has NONE of the phase functions declared. Sure it has the python functions declared after the metadata but between "metadata and not-present" ?? => end of ebuild. > > > Are the ebuilds "perfect"? No. > > It the ebuild good enough? Yes > > "Good enough" is not sufficient when we're dealing with an ebuild that has > been repeatedly subject to negligence and that has been last-rited twice > already. When you've already imposed significant work on others, it is only > expected that others expect much more than the absolutely minimal effort > from you. Last time upstream were slackers, and antarus fixed many of their issues (not all, which lead to not being keyworded). The state of the fixed ebuild had minimal impact to any deployment: - copyright year off-by-one - NOT install the tests anymore - wrong comments about tests - dependency dev-python/pytest-cov could be tightened to dev-python/pytest - line distutils_enable_tests pytest in the "wrong" place with no clearly stated style guide rule or automated QA about where it should be - distutils-r1_python_compile PEP517 behavior changed and it ignores arguments now, so --verbose did nothing - ebuild explicitly didn't install the python tests, which isn't against any QA rules I find it interesting that you didn't respond to my surprise that QA rules of the "Python Guide" was now apparently mandatory for all ebuilds using distutils eclasses, when there's been no clear announcements about that.
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=61869d38dc6718e9ab3a79c8ff5f08600551f2f9 commit 61869d38dc6718e9ab3a79c8ff5f08600551f2f9 Author: Robin H. Johnson <robbat2@gentoo.org> AuthorDate: 2024-02-01 06:38:33 +0000 Commit: Robin H. Johnson <robbat2@gentoo.org> CommitDate: 2024-02-01 07:54:45 +0000 net-nds/nsscache: QA fixes Bug: https://bugs.gentoo.org/923061 Signed-off-by: Robin H. Johnson <robbat2@gentoo.org> net-nds/nsscache/nsscache-0.49.ebuild | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
First of all, I am very sorry about my abrasive behavior towards you. I shouldn't have let my personal problems influence the way I'm interacting with others. (In reply to Robin Johnson from comment #3) > Can you please clarify which QA issues it was supposed to detect? At the very least, it detects outdated copyright and PYTHON_COMPAT values, incorrect commit messages and probably more. > And you said did explicitly say "pkgcheck check --commit"; which doesn't > seem to exist. I'm sorry, that was a typo. I meant `pkgcheck scan --commit`. > Are you really going to tell me that every Gentoo developer checks that > every file in every package matches what upstream declares as the license? I don't know what other developers do. I know what I do, and what is the correct thing to do. > This was ALREADY clarified in the comments, why are you continuing to > complain about it? I'm sorry, I merely was answering your question from the reply, without consulting the ebuild. > Many ebuilds have distutils_enable_tests as the last lines. > But really, putting it in the document would have no lasting impact unless > there is automation/tooling to detect and possibly fix that it's not ideal. > QA check would be relatively simple line number of "^distutils_enable_tests" > must be less than line number of (phase_functions, default to positive > infinity), AND greater than line number of (key metadata variables) I will request a check for that. It simply never occurred to me that this would be a problem because it was always clear to me that metadata should come before phase functions, and this function is a wrapper to set metadata. > And if you wanted to further muddy the waters, the nsscache ebuild has NONE > of the phase functions declared. Sure it has the python functions declared > after the metadata but between "metadata and not-present" ?? => end of > ebuild. These are consistently referred to as sub-phase functions. > I find it interesting that you didn't respond to my surprise that QA rules > of the "Python Guide" was now apparently mandatory for all ebuilds using > distutils eclasses, when there's been no clear announcements about that. Are you saying that ebuilds aren't supposed to follow well-defined API? All the API-impacting eclass changes were submitted for review, and I'm pretty sure the existence of Python Guide was announced as well. I don't understand why you're undermining the major effort of providing good documentation towards working in a non-trivial ecosystem.
(In reply to Michał Górny from comment #5) > First of all, I am very sorry about my abrasive behavior towards you. I > shouldn't have let my personal problems influence the way I'm interacting > with others. Apology accepted. Are there any other QA issues with this package that you are concerned about, so I can fix those? I did an evaluation to find possible alternatives for the function, should this package be removed: There's nothing else open-source and even close to maintained that I could find, short of a much larger migration to sssd and it's massive dependency tree. > > (In reply to Robin Johnson from comment #3) > > Can you please clarify which QA issues it was supposed to detect? > > At the very least, it detects outdated copyright and PYTHON_COMPAT values, > incorrect commit messages and probably more. Of that (short) list, it should have caught my off-by-one copyright header only. I went to verify the pkgcheck source, and I think there is a bug somewhere. ''' --commits [tree-ish] determine scan targets from unpushed commits ''' So I passed the tree hash for the ebuild where I bumped this (and should have the QA faults); got nothing; passing the commit gave me the same UnstableOnly info as before. ''' $ pkgcheck scan --commits daec2334caf4b2085bfc68996e88e70d0c5bad19 (no output) $ pkgcheck scan --commits 854aa3695b55ac48d199e269e0029d0c68363e86 net-nds/nsscache UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ 0.49 ] $ pkgcheck scan --commits 854aa3695b55ac48d199e269e0029d0c68363e86^ www-client/httrack UnstableOnly: for arch: [ arm64 ], all versions are unstable: [ 3.49.5 ] PotentialStable: version 3.49.5: slot(0), stabled arches: [ amd64, ppc, x86 ], potential: [ ~arm64 ] $ pkgcheck scan --commits 854aa3695b55ac48d199e269e0029d0c68363e86^..854aa3695b55ac48d199e269e0029d0c68363e86 net-nds/nsscache UnstableOnly: for arches: [ amd64, x86 ], all versions are unstable: [ 0.49 ] ''' There should absolutely be a way to check what was introduced in a commit, AFTER that commit has been pushed (or maybe my ebuild just wasn't bad enough to trigger issues). > > > And you said did explicitly say "pkgcheck check --commit"; which doesn't > > seem to exist. > > I'm sorry, that was a typo. I meant `pkgcheck scan --commit`. > > > Are you really going to tell me that every Gentoo developer checks that > > every file in every package matches what upstream declares as the license? > > I don't know what other developers do. I know what I do, and what is the > correct thing to do. I entirely agree the optimally correct thing is to always analyze every single source file and see that upstream knows what they are doing ;-). But most of the time, taking upstream's word for it is not going to have a significant difference in the outcome. And even if it's wrong, it can be fixed easily. > > Many ebuilds have distutils_enable_tests as the last lines. > > But really, putting it in the document would have no lasting impact unless > > there is automation/tooling to detect and possibly fix that it's not ideal. > > QA check would be relatively simple line number of "^distutils_enable_tests" > > must be less than line number of (phase_functions, default to positive > > infinity), AND greater than line number of (key metadata variables) > > I will request a check for that. It simply never occurred to me that this > would be a problem because it was always clear to me that metadata should > come before phase functions, and this function is a wrapper to set metadata. I like it as a QA checks; introduce some annotations into eclass that says "this function is used to set metadata", and then apply automated checks to ensure it's before any of the known phase (or sub-phase as you point out) functions. > > I find it interesting that you didn't respond to my surprise that QA rules > > of the "Python Guide" was now apparently mandatory for all ebuilds using > > distutils eclasses, when there's been no clear announcements about that. > > Are you saying that ebuilds aren't supposed to follow well-defined API? All > the API-impacting eclass changes were submitted for review, and I'm pretty > sure the existence of Python Guide was announced as well. I don't > understand why you're undermining the major effort of providing good > documentation towards working in a non-trivial ecosystem. Yes, I entirely agree that the Python Guide was announced, quite a long time ago now (in the earlier pre-git form). What the Guide does not have, is the nuance on what parts are "MUST"/"SHOULD"/"MAY" in terms of QA. E.g. where distutils_enable_tests should be. Good QA rules should be enforcable in an automated fashion, without forcing everybody to memorize any documents (including the Python Guide). To qualify for a "MUST", ideally it should be something that would significantly affect the outcome, possible to check in an automated fashion (ideally via static analysis) and you get a hard QA error when it's statically checked. Hard blocks. For things that that would significantly affect the outcome, but cannot be statically checked, the ebuild should fail with a hard error. So that it doesn't get installed. At the next level down, of SHOULD, is everything that gets you warnings. Somebody (forgot who) was actually asking me if we could start enforcing static checks on push, and that's entirely within the realm of possible: to take the subjectiveness and toil out of QA workflows, and leave the humans to deal with people who bypass the tools for the "MUST" category without good reasons.
Thanks so far. From a quick look: 1. (minor) technically test dependencies are BDEPEND. 2. distutils_enable_tests add a dep on pytest already. 3. (minor) The modern ebuild skel puts S immediately below SRC_URI, as the two are generally expected to match (i.e. the tarball name and the directory inside it). 4. sed calls are missing '|| die' 5. python_compile() { distutils-r1_python_compile; } is redundant.
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=f4e12ad13f410d8c4354e94207234d0007b8bcaa commit f4e12ad13f410d8c4354e94207234d0007b8bcaa Author: Robin H. Johnson <robbat2@gentoo.org> AuthorDate: 2024-07-08 16:07:50 +0000 Commit: Robin H. Johnson <robbat2@gentoo.org> CommitDate: 2024-07-08 16:17:04 +0000 net-nds/nsscache: QA improvements Signed-off-by: Robin H. Johnson <robbat2@gentoo.org> Closes: https://bugs.gentoo.org/923061 .../{nsscache-0.49.ebuild => nsscache-0.49-r1.ebuild} | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
