Skip to content

Drop sage-cleaner interface#40185

Merged
vbraun merged 2 commits intosagemath:developfrom
antonio-rojas:sage-cleaner
Jun 14, 2025
Merged

Drop sage-cleaner interface#40185
vbraun merged 2 commits intosagemath:developfrom
antonio-rojas:sage-cleaner

Conversation

@antonio-rojas
Copy link
Copy Markdown
Contributor

This doesn't work with meson, since sage-cleaner is not available, and breaks parallel doctesting

It is only used in interfaces/quit.py, where we can replace it by the existing temporary dir mechanism

Fixes #39873

This doesn't work with meson, since sage-cleaner is not available, which breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the existing temporary dir mechanism
d = os.path.join(DOT_SAGE, "temp", HOSTNAME, str(os.getpid()))
os.makedirs(d, exist_ok=True)
return os.path.join(d, "spawned_processes")
from sage.misc.temporary_file import tmp_dir
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it much work to migrate this to the built-in python tempdir?

(Triggered by the fact the last PR that touched the cleaner interface #33213 notes "Afterward, the custom functions tmp_dir() and tmp_filename() can be deprecated in favor of tempfile.TemporaryDirectory() and tempfile.NamedTemporaryFile().")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't been able to make it work. If I use TemporaryDirectory then the directory seems to be deleted immediately, unless I use delete=False, in which case it is not removed on sage exit.

This seems to be similar to the parallel doctesting issues we found on Python 3.13 upgrade, see #39201 (cc @tornaria)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for checking!

@antonio-rojas
Copy link
Copy Markdown
Contributor Author

antonio-rojas commented May 31, 2025

cc @orlitzky for input since I just noticed his previous sage-cleaner removal attempt at #33213 (which didn't go through at the end)

Note that this doesn't actually remove the sage-cleaner script, and in particular the sage script installed by setuptools still uses it. OTOH the meson sage script never called sage-cleaner, and I haven't seen any issue in a few weeks of testing: everything (temporary dirs and spawned subprocesses) is properly cleaned up on exit. I tried crashing a sage process which had gap and gp subprocesses running and they were killed too (temporary files weren't deleted, but I don't think anybody should expect a crashing program to clean up its temporary files)

Copy link
Copy Markdown
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side.

@orlitzky
Copy link
Copy Markdown
Contributor

orlitzky commented Jun 1, 2025

temporary files weren't deleted, but I don't think anybody should expect a crashing program to clean up its temporary files)

A lot of these can be "fixed" by #36322. The sage tmp_filename() and tmp_dir() functions leave the files/directories until sage exits cleanly. Then if sage does not exit cleanly, stuff gets left behind. But often, a temporary file/directory is much more temporary than that. And in that case, using the python tempfile module directly lets us clean up earlier (when we are done with the file/directory), making it irrelevant if sage eventually crashes. In any case, this is a minor issue IMO. I've been in no hurry to work on issue #36322 because the replacement code will be a lot nicer when we can assume python-3.12.

I also do not think there are any major problems with leaving processes behind. It depends on the program, but e.g. GAP and maxima are well-behaved, and it isn't really a sage problem if some optional package doesn't handle SIGPIPE correctly. Sage (the library) can still be used to launch other processes... but again, if the user wants to launch a daemon using popen or something like that... okay? I think the least surprising behavior is if the sage shell acts like the python shell in this regard (i.e. no sage-cleaner).

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 2, 2025

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

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 6, 2025
sagemathgh-40185: Drop sage-cleaner interface
    
This doesn't work with meson, since `sage-cleaner` is not available, and
breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the
existing temporary dir mechanism

Fixes sagemath#39873
    
URL: sagemath#40185
Reported by: Antonio Rojas
Reviewer(s): Antonio Rojas, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 8, 2025
sagemathgh-40185: Drop sage-cleaner interface
    
This doesn't work with meson, since `sage-cleaner` is not available, and
breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the
existing temporary dir mechanism

Fixes sagemath#39873
    
URL: sagemath#40185
Reported by: Antonio Rojas
Reviewer(s): Antonio Rojas, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 9, 2025
sagemathgh-40185: Drop sage-cleaner interface
    
This doesn't work with meson, since `sage-cleaner` is not available, and
breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the
existing temporary dir mechanism

Fixes sagemath#39873
    
URL: sagemath#40185
Reported by: Antonio Rojas
Reviewer(s): Antonio Rojas, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 9, 2025
sagemathgh-40185: Drop sage-cleaner interface
    
This doesn't work with meson, since `sage-cleaner` is not available, and
breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the
existing temporary dir mechanism

Fixes sagemath#39873
    
URL: sagemath#40185
Reported by: Antonio Rojas
Reviewer(s): Antonio Rojas, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 9, 2025
sagemathgh-40185: Drop sage-cleaner interface
    
This doesn't work with meson, since `sage-cleaner` is not available, and
breaks parallel doctesting

It is only used in `interfaces/quit.py`, where we can replace it by the
existing temporary dir mechanism

Fixes sagemath#39873
    
URL: sagemath#40185
Reported by: Antonio Rojas
Reviewer(s): Antonio Rojas, Tobias Diez
@vbraun vbraun merged commit c8c30f5 into sagemath:develop Jun 14, 2025
23 of 26 checks passed
@antonio-rojas antonio-rojas deleted the sage-cleaner branch June 14, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Meson build: random failures in doctesting due to missing sage-cleaner

4 participants