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 8290ad7; 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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

back up/restore conftest.py #39363

back up/restore conftest.py #39363

dimpase commented Jan 22, 2025

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

user202729 Jan 24, 2025

user202729 Jan 24, 2025

dimpase Jan 25, 2025

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

dimpase commented Jan 23, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading