Bug 243937 - lang/python38: Fails to package in certain conditions due to DISABLED_EXTENSIONS not working
Summary: lang/python38: Fails to package in certain conditions due to DISABLED_EXTENSI...
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-python (Nobody)
Keywords: buildisok, needs-patch, needs-qa
Depends on:
Reported: 2020-02-06 19:28 UTC by John Hein
Modified: 2021-06-15 17:05 UTC (History)
5 users (show)

See Also:
bugzilla: maintainer-feedback? (python)
jcfyecrayz: maintainer-feedback+

[patch] fix broken disable modules (v0) (1.45 KB, patch)
2020-02-06 19:28 UTC, John Hein
no flags Details | Diff
[patch] fix broken disable modules (v1) (995 bytes, patch)
2020-02-06 19:29 UTC, John Hein
no flags Details | Diff
[patch] fix broken disable modules (v2) (1019 bytes, patch)
2020-02-07 04:01 UTC, John Hein
no flags Details | Diff
[patch] fix broken disable modules - py38+39 (v3) (2.12 KB, patch)
2021-06-06 01:07 UTC, John Hein
no flags Details | Diff
[patch] fix broken disable modules - py37+38+39 (v4) (5.72 KB, patch)
2021-06-08 19:16 UTC, John Hein
jcfyecrayz: maintainer-approval? (python)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2020-02-06 19:28:22 UTC
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).
Comment 1 John Hein 2020-02-06 19:29:51 UTC
Created attachment 211421 [details]
[patch] fix broken disable modules (v1)

Here's another version using Setup.local.  Pick your poison.
Comment 2 John Hein 2020-02-06 19:30:58 UTC
Neither patch should require a PORTREVISION bump.
Comment 3 John Hein 2020-02-07 02:35:34 UTC

 - v1 patch ok in poudriere 11-stable/amd64.

 - portlint:

   That patch (v1) should use ${PRINTF} vs. printf.  Noticed by portlint.
Comment 4 John Hein 2020-02-07 04:01:37 UTC
Created attachment 211437 [details]
[patch] fix broken disable modules (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.
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2020-02-07 10:52:38 UTC
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.
Comment 6 John Hein 2020-02-07 11:44:26 UTC
(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.
Comment 7 John Hein 2020-02-07 12:10:55 UTC
(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.
Comment 8 Automation User 2020-04-10 01:28:36 UTC
Build info is available at https://gitlab.com/swills/freebsd-ports/pipelines/134601403
Comment 9 John Hein 2020-07-15 18:16:08 UTC

This still works as intended with 3.8.4, by the way.
Comment 10 John Hein 2020-07-15 18:20:44 UTC
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).
Comment 11 Enji Cooper freebsd_committer 2021-03-10 19:07:40 UTC
koobs@: could this please be committed? We have a hack in our codebase (Isilon OneFS) that effectively does this.
Comment 12 John Hein 2021-06-06 01:03:26 UTC
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.
Comment 13 John Hein 2021-06-06 01:07:23 UTC
Created attachment 225585 [details]
[patch] fix broken disable modules - py38+39 (v3)

Update patch to include python39.

No change to patch for python38.

 portlint - ok (no new warnings)
 testport - ok (11/stable amd64)
Comment 14 John Hein 2021-06-06 01:09:44 UTC
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.
Comment 15 Kubilay Kocak freebsd_committer freebsd_triage 2021-06-06 01:18:20 UTC
(In reply to John Hein from comment #14)

Thanks for the update John, will re-review this today
Comment 16 Kubilay Kocak freebsd_committer freebsd_triage 2021-06-07 01:18:20 UTC
(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.
Comment 17 John Hein 2021-06-08 14:31:32 UTC
(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
Comment 18 John Hein 2021-06-08 19:16:32 UTC
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.

 testport - ok (python37, 38, 39 default options and also all three tested with WITHOUT_NIS=1) [stable/11 amd64]
 portlint - ok (no new warnings)
Comment 19 John Hein 2021-06-08 19:48:21 UTC
(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).
Comment 20 John Hein 2021-06-15 17:05:29 UTC
Flip feedback flag to indicate feedback was provided (last week).