back up/restore conftest.py #39363

dimpase · 2025-01-22T00:11:49Z

this will fix #39357

📝 Checklist

[ x] The title is concise and informative.
[x ] The description explains in detail what this PR is about.
[x ] I have linked a relevant issue or discussion.
I have created tests covering the changes.
[ x] I have updated the documentation and checked the documentation preview.

⌛ Dependencies

as autoconf's generated configure clobbers conftest*, we are saving conftest.py from it by making a backup copy and then restoring it after the real configure was run.

dimpase · 2025-01-22T00:13:10Z

@vbraun - please have a look whether this doesn't break anything in your machinery

dimpase · 2025-01-22T03:19:21Z

@orlitzky - can you have a look?

dimpase · 2025-01-22T14:58:01Z

CI is failing in building docker images with

#23 66.22 cp: cannot stat 'configure_wrapper': No such file or directory

No idea what wrapper this is. @kwankyu - do you know?

jhpalmieri · 2025-01-22T17:20:48Z

CI is failing in building docker images with
#23 66.22 cp: cannot stat 'configure_wrapper': No such file or directory
No idea what wrapper this is. @kwankyu - do you know?

I may not be understanding this correctly, but isn't it just referring to the file, newly created by this PR, called "configure_wrapper"?

dimpase · 2025-01-22T20:13:15Z

Perhaps I've broken make configure - something that is used by the docker creation. I'm not sure this make target should be preserved - it's not standard, people can and should instead run ./bootstrap directly.

kwankyu · 2025-01-23T00:25:21Z

No idea what wrapper this is. @kwankyu - do you know?

No idea either.

Perhaps I've broken make configure - something that is used by the docker creation. I'm not sure this make target should be preserved - it's not standard, people can and should instead run ./bootstrap directly.

If something is broken, the standard way is to fix it (after understanding how it went broken), not to remove it.

dimpase · 2025-01-23T00:45:28Z

No idea what wrapper this is. @kwankyu - do you know?

No idea either.

Perhaps I've broken make configure - something that is used by the docker creation. I'm not sure this make target should be preserved - it's not standard, people can and should instead run ./bootstrap directly.

If something is broken, the standard way is to fix it (after understanding how it went broken), not to remove it.

Creating configure scripts using make is highly non-standard, and totally unneeded. I don't know why we're stuck with this for so long. It makes the build system even more complicated than necessary - because normally speaking makefiles are built by ./configure, not the other way around. So we're creating a chicken vs egg situation for no reason at all.

Anyhow, my latest attempt shows that it's not the issue. The issue is that the docker does not find configure_wrapper, and for the life of me I don't understand why. This file is a new file present in this branch, the branch we are testing!

https://github.com/sagemath/sage/actions/runs/12916470243/job/36020720010?pr=39363#step:13:517

orlitzky · 2025-01-23T01:31:15Z

I saw your post on the autoconf list and had been planning to try this. It's worse than you say:

The fact that ./configure will remove all conftest* files has been documented for many many years, so it was an interesting choice by pytest to name their file conftest.py.
Not only does autoconf remove conftest* files, it removes them with rm -rf, so we can't fix this with e.g. chmod a-w conftest.py.
Not only does autoconf rm -rf them, it does so in a trap handler to ensure that they are removed as the very last action taken by ./configure. So we can't e.g. move conftest.py to _conftest.py and then symlink it at the end of ./configure.

dimpase · 2025-01-23T01:52:02Z

I saw your post on the autoconf list and had been planning to try this. It's worse than you say:
I mentioned somewhere that I got a reply to my post on the autoconf list, too, basically confirming your observations.

it was an interesting choice by pytest to name their file conftest.py.
There aren't many python packages around built with autotools. (cpython itself was like this IIRC, but, well...)

dimpase · 2025-01-23T01:53:12Z

Are we in one of these situations when our docker-based CI won't work until this PR is merged in develop? @kwankyu ?

kwankyu · 2025-01-23T02:19:13Z

I have no idea what you are asking... What is "these situations"? I was not following the present issue and have no background understanding.

kwankyu · 2025-01-23T02:32:33Z

By the way, I do not object to changing "make configure" with "./bootstrap" for now if it helps. But removing the existing feature "make configure" as you seem to advocate is another matter.

dimpase · 2025-01-23T04:43:06Z

I have no idea what you are asking... What is "these situations"? I was not following the present issue and have no background understanding.

basically, running ./configure removes all the files matching conftest* in Sageroot, but pytest needs a file named conftest.py in there.
There is basically no way to modify ./configure to avoid this, so we need to run it in a wrapper which will restore conftest.py after ./configure is run.

to preserve our workflow, we rename configure to real_configure and install configure_wrapper as ./configure. This is done in ./bootstrap. All works just fine on a local machine.
But the part of CI which creates a docker container breaks down while running ./bootstrap,
as it can't find configure_wrapper - which is a new file introduced in this PR.
IMHO it's a bug in CI, but I don't see where.

user202729 · 2025-01-24T04:00:54Z

Probably #39373 , let's see.

github-actions · 2025-01-24T07:08:45Z

Documentation preview for this PR (built with commit f6d72ba; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

dimpase · 2025-01-24T17:34:13Z

@user202729 - CI passes (modulo unrelated doctest failures on some platforms). Can you give this a positive review?

jhpalmieri · 2025-01-24T18:30:17Z

By the way, I do not object to changing "make configure" with "./bootstrap" for now if it helps. But removing the existing feature "make configure" as you seem to advocate is another matter.

I would not have any objections, either, but it is a publicly advertised way to install Sage — it's in the top-level README.md, for example — and so make configure would need to go through the usual deprecation process, we can't just delete it.

dimpase · 2025-01-24T18:37:30Z

By the way, I do not object to changing "make configure" with "./bootstrap" for now if it helps. But removing the existing feature "make configure" as you seem to advocate is another matter.

I would not have any objections, either, but it is a publicly advertised way to install Sage — it's in the top-level README.md, for example — and so make configure would need to go through the usual deprecation process, we can't just delete it.

This goes without saying, certainly. The thing is that make configure isn't even an explicit make target, it's using the generic target for spkgs. I can make an explicit target which just runs ./bootstrap.

user202729

haven't actually reviewed it carefully, just a few minor changes then should be good

user202729 · 2025-01-24T22:49:13Z

configure_wrapper

@@ -0,0 +1,4 @@
+#! /bin/sh
+cp conftest.py bak_conftest.py
+./real_configure $@


Wouldn't it be safer to change $@ to "$@" to avoid arguments with spaces being broken up (not sure if it's portable, at least it works in bash)

also does the real_configure script support running it in a different directory? if yes then it may be a good idea to also support it by changing it to "$(dirname "$0")"/real_configure

user202729 · 2025-01-24T22:49:55Z

docker/Dockerfile

@@ -196,7 +196,7 @@ ARG MAKEFLAGS="-j2"
 ENV MAKEFLAGS $MAKEFLAGS
 ARG SAGE_NUM_THREADS="2"
 ENV SAGE_NUM_THREADS $SAGE_NUM_THREADS
-RUN make configure
+RUN ./bootstrap


does make configure still work? (if yes maybe leave it alone to be conservative, if not maybe we need to fix it)

yes, make configure should still work. However, I don't know how, and trying to figure it out gives me a headache. OK, let's revert this change and see.

.github/workflows/build.yml

This reverts commit 48a81bd.

jhpalmieri · 2025-01-27T20:08:31Z

As an alternate approach, can we rename conftest.py to something_else.py and use pytest -p something_else? (See first comment at https://stackoverflow.com/questions/63236001/in-pytest-how-can-i-specify-a-different-conftest-py-file-to-use, and also https://discuss.python.org/t/pytests-conftest-py-naming-in-compatibility-with-autoconf/78085/11.) I would rather mess with pytest and leave configure unchanged, given the choice.

user202729 · 2025-01-28T03:34:44Z

Thinking about it, maybe that can be automated with addopts in pytest.ini?

dimpase · 2025-01-28T04:12:00Z

everybody is welcome to provide a better fix. I'm a bit tired of pytest now.

user202729 · 2025-01-28T04:40:21Z

To be fair, naming it something else might lead to some confusion. If this works we might as well merge it in until some other issue comes up.

orlitzky · 2025-01-28T12:54:13Z

For the current approach, the final rename should probably come in a trap handler. Otherwise a Ctrl-C will leave things inconsistent. (But I also would prefer to mess with pytest if someone figures out how to do it.)

jhpalmieri · 2025-01-28T17:50:51Z

In my opinion, the cleanest solution would be to back out the changes that caused the original problem, namely #39206. From the description there, it sounds like pytest is not ready for widespread use with Sage, whereas just about all developers use ./configure frequently.

I don't know much about pytest, but aren't all of our doctests in src/? If so, it would make sense to put testing-related materials there. Either do cd src; pytest or put a configuration file there and run pytest -p ...

tobiasdiez · 2025-01-28T18:12:15Z

In my opinion, the cleanest solution would be to back out the changes that caused the original problem, namely #39206. From the description there, it sounds like pytest is not ready for widespread use with Sage, whereas just about all developers use ./configure frequently.

That's the status-quo. In the future, however, everyone uses pytest (instead of our custom-made doctest runner) and meson (instead of configure + make). We can of course discuss how close we are to this future, but in any case we should work towards this goal and not away from it. So in the current context, fix configure and keep pytest as close to the vanilla pytest experience as possible.

(I'm also not sure if the pytest -p solution works, as conftest.py is not actually a proper plugin.)

The changes here look good to me, maybe move the wrapper to the build directory if possible.

dimpase · 2025-01-28T18:42:11Z

pytest had made an unfortunate choice of names for their config files. pytest is increasingly used more and more in Sage and their dependencies.

dimpase · 2025-01-28T21:44:38Z

@tobiasdiez - did you try creating a directory tests/ and moving toplevel conftest.py there?

tobiasdiez · 2025-01-29T12:23:13Z

No, I've not tried this idea. The reason why the conftest is in the root now, is because there were a couple of issues with importing the doctests. Roughly speaking, src is the root for all sage imports but you want to invoke pytest from SAGE_ROOT; this difference lead to a couple of issues and the only reliable solution I could find was to move the conftest file to the root.

dimpase · 2025-01-31T23:46:56Z

@tobiasdiez if I try running pytest on a Sage installation built in the usual way (no meson), all I get is an import error:

$ ./sage --pytest
================================================================================================================================== test session starts ===================================================================================================================================
platform linux -- Python 3.12.5, pytest-8.3.2, pluggy-1.5.0
rootdir: /mnt/opt/Sage/sage-clang/src
configfile: ../pyproject.toml
plugins: xdist-3.6.1, mock-3.14.0, hypothesis-6.124.2, anyio-4.4.0
collected 32904 items / 1 error / 12 skipped                                                                                                                                                                                                                                             

========================================================================================================================================= ERRORS =========================================================================================================================================
___________________________________________________________________________________________________________________ ERROR collecting sage/all__sagemath_categories.py ____________________________________________________________________________________________________________________
src/sage/all__sagemath_categories.py:6: in <module>
    from sage.rings.all__sagemath_categories import *
E   ModuleNotFoundError: No module named 'sage.rings.all__sagemath_categories'
================================================================================================================================ short test summary info =================================================================================================================================
ERROR src/sage/all__sagemath_categories.py - ModuleNotFoundError: No module named 'sage.rings.all__sagemath_categories'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
======================================================================================================================== 12 skipped, 1 error in 222.20s (0:03:42) ========================================================================================================================
dima@hilbert /mnt/opt/Sage/sage-clang $

Is this a bug? Or I do something wrong? sage.rings.all__sagemath_categories ought to be removed one way or another.

tobiasdiez · 2025-02-01T00:36:02Z

This seems to be a genuine error, there is no sage.rings.all__sagemath_categories only https://github.com/sagemath/sage/blob/develop/src/sage/rings/all__sagemath_objects.py, right?

Looking at https://github.com/search?q=repo%3Asagemath%2Fsage%20all__sagemath_categories&type=code all these _categories things can just be deleted without problem as they are not used (which is probably the reason why the above error was not noticed before).

dimpase · 2025-02-01T17:17:54Z

This seems to be a genuine error, there is no sage.rings.all__sagemath_categories only https://github.com/sagemath/sage/blob/develop/src/sage/rings/all__sagemath_objects.py, right?

Looking at https://github.com/search?q=repo%3Asagemath%2Fsage%20all__sagemath_categories&type=code all these _categories things can just be deleted without problem as they are not used (which is probably the reason why the above error was not noticed before).

simply deleting the offending import makes pytest run OK. (about 500 failures out of about 5000 tests, but that's not unexpected, right?)

I also tried moving conftest.py to src/.
Then tests pass with the same error count (but it took about 2 times longer, for some reason)

dimpase added 2 commits January 20, 2025 17:27

correctly document location of conftest.py for pytest

503a0a0

wrap ./configure in a wrapper saving conftest.py

c68d07a

as autoconf's generated configure clobbers conftest*, we are saving conftest.py from it by making a backup copy and then restoring it after the real configure was run.

github-actions bot added the s: needs review label Jan 22, 2025

dimpase requested review from jhpalmieri, tobiasdiez and vbraun January 22, 2025 00:12

dimpase added c: build: configure p: blocker / 1 labels Jan 22, 2025

add the wrapper and real_configure to configure tarball

08aecad

use ./bootstrap in building docker container

48a81bd

move wrapper creation to the better place

7ad95b9

user202729 and others added 2 commits January 24, 2025 00:14

also ADD configure_wrapper and conftest.py

776464d

add comments suggested on #39373i by user202729

7659ca0

user202729 reviewed Jan 24, 2025

View reviewed changes

dimpase added 2 commits January 24, 2025 22:12

Revert "use ./bootstrap in building docker container"

ff0f316

This reverts commit 48a81bd.

typo fix

8290ad7

dimpase mentioned this pull request Jan 27, 2025

Test that the Sage git repo is clean. #39392

Open

5 tasks

Merge branch 'develop' into conftest_py_fix

f6d72ba

back up/restore conftest.py #39363

Are you sure you want to change the base?

back up/restore conftest.py #39363

Conversation

dimpase commented Jan 22, 2025

📝 Checklist

⌛ Dependencies

dimpase commented Jan 22, 2025

dimpase commented Jan 22, 2025

dimpase commented Jan 22, 2025

jhpalmieri commented Jan 22, 2025

dimpase commented Jan 22, 2025

kwankyu commented Jan 23, 2025

dimpase commented Jan 23, 2025 • edited Loading

orlitzky commented Jan 23, 2025

dimpase commented Jan 23, 2025

dimpase commented Jan 23, 2025

kwankyu commented Jan 23, 2025

kwankyu commented Jan 23, 2025

dimpase commented Jan 23, 2025

user202729 commented Jan 24, 2025

github-actions bot commented Jan 24, 2025 • edited Loading

dimpase commented Jan 24, 2025

jhpalmieri commented Jan 24, 2025

dimpase commented Jan 24, 2025

user202729 left a comment

Choose a reason for hiding this comment

user202729 Jan 24, 2025

Choose a reason for hiding this comment

user202729 Jan 24, 2025

Choose a reason for hiding this comment

dimpase Jan 25, 2025

Choose a reason for hiding this comment

jhpalmieri commented Jan 27, 2025

user202729 commented Jan 28, 2025

dimpase commented Jan 28, 2025

user202729 commented Jan 28, 2025

orlitzky commented Jan 28, 2025

jhpalmieri commented Jan 28, 2025

tobiasdiez commented Jan 28, 2025

dimpase commented Jan 28, 2025

dimpase commented Jan 28, 2025

tobiasdiez commented Jan 29, 2025

dimpase commented Jan 31, 2025

tobiasdiez commented Feb 1, 2025

dimpase commented Feb 1, 2025

dimpase commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading