Created attachment 211420 [details] [patch] fix broken disable modules (v0) make check-plist gives errors for lang/python38 ... ====> Checking for pkg-plist issues (check-plist) ===> Parsing plist ===> Checking for items in STAGEDIR missing from pkg-plist Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_gdbm.so Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_sqlite3.so Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_tkinter.so ===> Checking for items in pkg-plist which are not in STAGEDIR ===> Error: Plist issues found. *** Error code 1 The attached patch fixes the check-plist error (and avoids building the modules saving a bit of build time) by using Modules/Setup. See also https://bugs.python.org/issue20210 The fix could also be applied to python37 (and then we could remove the files/patch-issue20210 and DISABLED_EXTENSIONS in Makefile).
Created attachment 211421 [details] [patch] fix broken disable modules (v1) Here's another version using Setup.local. Pick your poison.
Neither patch should require a PORTREVISION bump.
QA: - v1 patch ok in poudriere 11-stable/amd64. - portlint: That patch (v1) should use ${PRINTF} vs. printf. Noticed by portlint.
Created attachment 211437 [details] [patch] fix broken disable modules (v2) v2: - Use ${PRINTF} (as suggested by portlint) - Use DISABLED_EXTENSIONS in the printf so now the WITHOUT_NIS case works for python38, too (it doesn't before this patch for the same reason that disabling the other extensions did not work). Confirmed in poudriere with and without WITHOUT_NIS set.
Thank you for the report John Could you clarify if the origin of this issue was identified? Was it a port or upstream change that regressed the DISABLED_MODULES functionality? Are any other lang/python* versions affected in the same way? Also, if I understand correctly, these modules will build (and result in pkg-plist) errors, if the associated third party headers/libraries are installed (such as tkinter, gdbm, etc), is that correct.
(In reply to Kubilay Kocak from comment #5) python37 has patch-issue20210 to "solve" how to disable building/installing the extensions. That got repocopied from python36 and python35/34 before that. You probably already know this. I don't know why it was not pulled into python38. Maybe the patch no longer applied? I didn't check. But from reading the upstream issue 20210, it's clear they went another way than the original patch we've been carrying along. The *disabled* marker method in Modules/Setup* was committed (looks like it was committed in python37 in 2017). So I think using that going forward is the right way to go (instead of trying to resurrect the old patch which upstream decided not to use). I just read bug 241416. The extensions problem was known and deferred (until someone could develop the fix). So I guess it just hadn't gotten addressed yet. I think (but have not tried) that this patch would work for our latest python37. We could choose to apply it there as well or just live with the old patch as long as it still works. pro for the new patch in python37: future python37 updates might break the old patch. con argument: unnecessary work/thrash (wait until current flavor stops working for py37, if ever). In any case, that decision can be separated from this fix for py38. Re: your second question re: third party headers. I don't know if having gdbm, et. al., ports installed before building py38 affects whether those extensions are built or not. I suspect it does, but I have not done that test to confirm. In my environment, they were installed.
(In reply to John Hein from comment #6) I just tried poudriere with an unpatched python38 without having gdb, sqlite, tk ports installed. The extensions were not built, and thus no check-plist error, confirming our suspicions. WITHOUT_NIS=1 does not prevent building/installing nis.so, however. That's to be expected since (before the patch here) DISABLED_EXTENSIONS does nothing for py38 (which has nothing like patch-issue20210 in python37, and earlier, that will use DISABLED_EXTENSIONS). There is not a plist error since @comment in the plist just makes plist checking ignore the nis.so file in the staging area. So it's just a little extra wasted build time. But the current v2 patch here solves that, too.
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/134601403
ping. This still works as intended with 3.8.4, by the way.
And regarding 'needs-qa', this passed testing and works as intended with various options set in poudriere (lang/python38, databases/py-sqlite3, databases/py-gdbm, . x11-toolkits/py-tkinter and FLAVOR=py38).
koobs@: could this please be committed? We have a hack in our codebase (Isilon OneFS) that effectively does this.
This patch still applies for python38-3.8.10. And it is still needed. Reproduce with: % sudo poudriere bulk -i -C -v -v -j jailX -p portsX x11-toolkits/py-tkinter databases/gdbm databases/py-sqlite3 . . [00:02:28] Entering interactive test mode. Type 'exit' when done. root@jailX-portsX:~ # cd /usr/ports/lang/python38 root@jailX-portsX:~ # make clean stage check-plist . . ===> Checking for items in STAGEDIR missing from pkg-plist Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_gdbm.cpython-38.so Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_sqlite3.cpython-38.so Error: Orphaned: lib/python%%XYDOT%%/lib-dynload/_tkinter.cpython-38.so ===> Checking for items in pkg-plist which are not in STAGEDIR ===> Error: Plist issues found. *** Error code 1 And now that python39 is in the ports tree, it fails the same way.
Created attachment 225585 [details] [patch] fix broken disable modules - py38+39 (v3) Update patch to include python39. No change to patch for python38. QA: portlint - ok (no new warnings) testport - ok (11/stable amd64)
Since Modules/Setup* has been the way to disable modules for a while now (see previous comments), python3.10 will likely need this as well when it gets in the ports tree.
(In reply to John Hein from comment #14) Thanks for the update John, will re-review this today
(In reply to Kubilay Kocak from comment #15) John, could you please you apply your changes to all lang/python* ports that currently contain DISABLED_EXTENSIONS, and confirm QA passes for those too. Also, since packaging doesnt fail in all cases, I believe we should be bumping PORTREVISION for all of them too.
(In reply to Kubilay Kocak from comment #16) The changes are not applicable to python36 - the new Modules/Setup was not backported to python36. So we can't change how python36 disables extensions (unless perhaps the Modules/Setup support is backported manually, but I think it is not necessary to spend energy on that). It is not needed for python37 because the 'disabled_modules_list' hack works fine. But it does have the new *disabled* marker support in Modules/Setup (by inspection - not tested). As mentioned in the original comment, we could apply the same change to python37 and remove patch-issue20210. Since that should work and is quite simple, I will put together another patch rev which does so as an option. Just for historical trivia - it looks like the *disabled* marker support was available even in python-3.7.1
Created attachment 225650 [details] [patch] fix broken disable modules - py37+38+39 (v4) (In reply to John Hein from comment #17) Here's a patch that includes the change for python37 as well (and removes the now unnecessary patch-issue20210). python37, python38 and python39 all build without the disabled extensions and do not have check-plist errors. QA: testport - ok (python37, 38, 39 default options and also all three tested with WITHOUT_NIS=1) [stable/11 amd64] portlint - ok (no new warnings)
(In reply to Kubilay Kocak from comment #16) I forgot to answer the PORTREVISION comment. I don't think we need to bump PORTREVISION. There's no change to the package. The extra cruft in the staging directory is not included in the package. It's just that check-plist pedantically throws an error (and there are some extra unnecessary cpu cycles spent on building the extensions).
Flip feedback flag to indicate feedback was provided (last week).
Hi. Thanks and sorry about the delay on this PR. I've just opened this review, please take a look. I'll do more tests; I plan to push it in a day or two.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=a94d4b1005b1e93a27bcb9e4e794eeb13c991dd5 commit a94d4b1005b1e93a27bcb9e4e794eeb13c991dd5 Author: Danilo G. Baio <dbaio@FreeBSD.org> AuthorDate: 2021-07-07 02:04:38 +0000 Commit: Danilo G. Baio <dbaio@FreeBSD.org> CommitDate: 2021-07-08 01:55:17 +0000 lang/python*: Replace DISABLED_EXTENSIONS with Setup.local Currently, lang/python38 and lang/python39 don't honor DISABLED_EXTENSIONS because patch-issue20210 was removed when lang/python38 was added to the ports tree. patch-issue20210 is still present on lang/python36 and lang/python37. Building with poudriere is not affected because builds are executed in a clean environment. Setup.local is the more canonical and recommended method for customizing Python builds for shared extensions & third party libraries. Support for a *disabled* marker in Setup files was introduced in Python 3.7, so backport this fix to it to keep consistency in the ports tree. PR: 243358 [1] PR: 243937 [2] Reported by: ngie [1] Reported by: jcfyecrayz@liamekaens.com [2] Reported by: tuxillo (IRC) DPorts Reviewed by: koobs (python, maintainer) Approved by: koobs, dbaio (python, maintainer) MFH: 2021Q3 (build bugfix) Differential Revision: https://reviews.freebsd.org/D31086 lang/python37/Makefile | 7 ++- lang/python37/files/patch-issue20210 (gone) | 68 ----------------------------- lang/python38/Makefile | 7 ++- lang/python39/Makefile | 7 ++- 4 files changed, 18 insertions(+), 71 deletions(-)
A commit in branch 2021Q3 references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=42b74dbb33b8a8fa1fcd06ca376270fed30c839c commit 42b74dbb33b8a8fa1fcd06ca376270fed30c839c Author: Danilo G. Baio <dbaio@FreeBSD.org> AuthorDate: 2021-07-07 02:04:38 +0000 Commit: Danilo G. Baio <dbaio@FreeBSD.org> CommitDate: 2021-07-08 02:12:09 +0000 lang/python*: Replace DISABLED_EXTENSIONS with Setup.local Currently, lang/python38 and lang/python39 don't honor DISABLED_EXTENSIONS because patch-issue20210 was removed when lang/python38 was added to the ports tree. patch-issue20210 is still present on lang/python36 and lang/python37. Building with poudriere is not affected because builds are executed in a clean environment. Setup.local is the more canonical and recommended method for customizing Python builds for shared extensions & third party libraries. Support for a *disabled* marker in Setup files was introduced in Python 3.7, so backport this fix to it to keep consistency in the ports tree. PR: 243358 [1] PR: 243937 [2] Reported by: ngie [1] Reported by: jcfyecrayz@liamekaens.com [2] Reported by: tuxillo (IRC) DPorts Reviewed by: koobs (python, maintainer) Approved by: koobs, dbaio (python, maintainer) Differential Revision: https://reviews.freebsd.org/D31086 (cherry picked from commit a94d4b1005b1e93a27bcb9e4e794eeb13c991dd5) lang/python37/Makefile | 7 ++- lang/python37/files/patch-issue20210 (gone) | 68 ----------------------------- lang/python38/Makefile | 7 ++- lang/python39/Makefile | 7 ++- 4 files changed, 18 insertions(+), 71 deletions(-)
Committed, thanks!