-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-27640: Add --disable-test-modules configure option #23886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Parts of the testsuite are used by third party packages. Even if you don't want to install test modules, you should install those.
However that list might be incomplete. For packaging purposes I think it's better to remove those files which you don't want to ship after the installation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parts of the testsuite are used by third party packages. Even if you don't want to install test modules, you should install those.
This is what I identify as part of the test framework:
test/{libregrtest,support}
test/{ann_module,ann_module2,ann_module3}.py
test/{regrtest,test_support,init,main}.py
However that list might be incomplete.
For packaging purposes I think it's better to remove those files which you don't want to ship after the installation.
@doko42 Thanks for your review. After I installed the Python3.8 distribution for Ubuntu, it indeed has the regression test framework shipped but with all other test stuffs removed. So always shipping test framework skeleton regardless of the option value of --enable-test-modules could be a good idea. Could we do the same thing like Ubuntu does as shown below? Under the directory /usr/lib/python3.8/test, Ubuntu ships: In addition, Ubuntu doesn't ship test/{ann_module,ann_module2,ann_module3}.py. How are these used by 3rd-party? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://bugs.python.org/issue31904 is a meta-issue about VxWorks, but I would prefer to have a dedicated issue for this change.
The good news is that it already exists: https://bugs.python.org/issue27640 Please reuse this BPO.
@@ -0,0 +1 @@ | |||
Add --enable-test-modules option for configure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document it also in https://docs.python.org/dev/whatsnew/3.10.html#build-changes (Doc/whatsnew/3.10.rst).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
configure.ac
Outdated
@@ -5833,6 +5833,18 @@ else | |||
fi], | |||
[AC_MSG_RESULT(no)]) | |||
|
|||
# check whether to disable test modules | |||
AC_MSG_CHECKING(for --enable-test-modules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the default is to enable tests, I would prefer an option to disable tests. https://bugs.python.org/issue27640 proposed to add --disable-test-suite to configure.
Can you please elaborate the effect of the option? I understand that if it's used, it disables the compilation of test extension modules, and prevent to install tests in "make install". Also elaborate it in the NEWS and What's New entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below copied from configure help. When we define an option enable-test-modules, the counterpart disable-test-modules will be also defined automatically. That is, disable-test-modules is already defined. The existing option --enable-ipv6 is an example that is default as yes.
Optional Features: --disable-option-checking ignore unrecognized --enable/--with options --disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no) --enable-FEATURE[=ARG] include FEATURE [ARG=yes] --enable-universalsdk[=SDKDIR] create a universal binary build. SDKDIR specifies which macOS SDK should be used to perform the build, see Mac/README.rst. (default is no) --enable-framework[=INSTALLDIR] create a Python.framework rather than a traditional Unix install. optional INSTALLDIR specifies the installation path. see Mac/README.rst (default is no) --enable-shared enable building a shared Python library (default is no) --enable-profiling enable C-level code profiling with gprof (default is no) --enable-optimizations enable expensive, stable optimizations (PGO, etc.) (default is no) --enable-loadable-sqlite-extensions support loadable extensions in _sqlite module, see Doc/library/sqlite3.rst (default is no) --enable-ipv6 enable ipv6 (with ipv4) support, see Doc/library/socket.rst (default is yes if supported) --enable-big-digits[=15|30] use big digits (30 or 15 bits) for Python longs (default is system-dependent)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"When we define an option enable-test-modules, the counterpart disable-test-modules will be also defined automatically."
Ah ok. But you should document that the change adds the --disable-test-modules option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot run "autoconf": configure it outdated, it contains the old documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ah ok. But you should document that the change adds the --disable-test-modules option."
Done.
"You forgot run "autoconf": configure it outdated, it contains the old documentation."
Actually I did run it.
Makefile.pre.in
Outdated
@@ -1365,8 +1365,29 @@ maninstall: altmaninstall | |||
|
|||
# Install the library | |||
XMLLIBSUBDIRS= xml xml/dom xml/etree xml/parsers xml/sax | |||
LIBSUBDIRS= tkinter tkinter/test tkinter/test/test_tkinter \ | |||
tkinter/test/test_ttk site-packages test \ | |||
LIBSUBDIRS= tkinter \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you rewrite LIBSUBDIRS and TESTSUBDIRS, can you please sort these lists? You may even put one item per line, but you can keep groups of subdirectories of the same directory (like "venv venv/scripts venv/scripts/common venv/scripts/posix").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
The buildroot project has the following patch on Python which adds |
Co-authored-by: Victor Stinner <vstinner@python.org>
https://bugs.debian.org/944303 showed that python-typing-extensions uses the ann* modules. Now that's integrated in upstream cpython, but still shipped on PyPi: https://pypi.org/project/typing-extensions/. So yes, I think these could be removed as well. Please note that Debian/Ubuntu packaging doesn't remove any of these files, but just ships them in a separate binary package, e.g. libpython3.8-testsuite. |
@vstinner Regarding the regression test framework(see below for files and directories) what is your opinion? Keeping it or removing it entirely when disabling test suites? Looks buildroot removes it while Ubuntu keeps it. If we decide keeping it, some of the test extension modules(_testcapi and _testinternalcapi) have to be kept as well because test.support needs to import them.
|
I have made the requested changes; please review again |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
The Fedora package put the following files/directories in the python3-test binary RPM:
https://src.fedoraproject.org/rpms/python3.10/blob/master/f/python3.10.spec#_1429 |
With the PR and
With the PR and
Install in
Install in
Remove
Compare the content of the two directories:
I atttached the full pr23886_files.diff to https://bugs.python.org/issue27640 |
Good: this PR does not remove any file/directory by default (compared to the master branch). It works as expected.
|
On Fedora, the The
|
On Debian, Ubuntu or Fedora, it's just a matter of putting the right dependency on the libpython3.8-testsuite (python3-test on Fedora) package.
This PR is not intended for general Linux distributions like Debian or Fedora, but specialized embedded devices. Xavier created https://bugs.python.org/issue27640 when he worked on putting Python on Android. The buildroot is also intended to be used to produce an image for an embedded devices. The use case is to ship an image for end users who will not run any test but use a "finished" product. For developers, well, just don't use |
@@ -0,0 +1,3 @@ | |||
Added ``--enable-test-modules`` option to the ``configure`` script. If this | |||
option is set to no, ``make install`` will not install test suites. Also | |||
setup.py will not build test extension modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind to add the credit?
Patch by Xavier de Gaye, Thomas Petazzoni and Peixing Xin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-Authored-By: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Co-Authored-By: Xavier de Gaye <xdegaye@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
I tested again the PR, at commit 9e5dada. I rebased locally the PR on master. The PR works as expected! Install Python 3 times:
First test: ensure that the PR still copy exactly the same files: good, there is zero difference!
Second test: check that --disable-test-modules don't install tests. Good! Test C extension are not built nor installed and test subdirectories are not installed, as expected.
|
Thank you @pxinwr for your multiple updates, the final PR is much better. The change is now properly documented (NEWS entry, What's New in Python 3.10 entry), the option is properly documented directly in configure (it says exactly what it does), it's better than Xavier's initial patch and the Thoma's buildbroot patch since it also doesn't build test extensions. And overall, I think that it's a reasonable build option, it makes sense to not install tests on a small "embedded device". |
|
Added --disable-test-modules option to the configure script: don't build nor install test modules. Patch by Xavier de Gaye, Thomas Petazzoni and Peixing Xin. Co-Authored-By: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Co-Authored-By: Xavier de Gaye <xdegaye@gmail.com>
Test modules under Lib directory are used for internal CPython regression testing. Shipping them in formal release is not necessary. So to reduce package size and build and installation time, we should provide an option in configure to disable them.
https://bugs.python.org/issue27640