Remove sage_conf from sage-the-distro #40882
Conversation
|
Documentation preview for this PR (built with commit b806949; changes) is ready! 🎉 |
|
these tests should be adjusted/removed: |
|
Thanks, fixed now. |
|
Building from a clean checkout still fails, the one thing that this was supposed to fix
|
|
the build obviously works, no? a long doctest fails. |
|
@vbraun - can we fix the long tests in a follow up? |
|
Just fix it on this PR |
github-actions
bot
added
s: needs review
and removed
s: positive review
s: needs work
labels
|
I fail to see how this is related to the |
|
to make debugging easier, I suggest changing the test to, say of course the reason why this makes things easier to debug is that all stdout are swallowed when there's an exception, so we avoid the exception and manually check the returncode, even if the process fails we can still see what the stdout/stderr were. If you or @dimpase can't reproduce it, you'll still need to wait for @vbraun to run it though. |
tobiasdiez
force-pushed
the
remove_sage_conf_minimial
branch
from
7698a33 to
b806949
Compare
|
Did anybody here actually try building Sage from a clean repo checkout? Its not that hard and I would be surprised if you wouldn't hit the same test failure. And a big 👎 from me for just deleting the test |
|
In effect we are slaving away for a macOS buildbot environment, an environment nobody develops on. A typical macOS machine is such a mess that often a much easier than building Sage tasks fail. I have to tell my students to create a separate account to do such work. (this week one of them could not manage to build a C program linking to GMP because of, well, environment!) |
Use something like docker or podman on a fast machine to create, say, a Gentoo image with all the Sage packages installed. Then testing Sage-the-distro only takes about as long as testing sagelib and docbuild. I also imagine we should run our CI with such an image, I don't see why our CI always builds Sage distro from configurations missing lots of packages.
|
Incremental build, or build from a clean repo checkout? |
|
@vbraun after as clean as it gets |
|
I'm getting |
|
Also, do you have a global if value is None:
try:
import sage_conf
value = getattr(sage_conf, key, None)
except ImportError:
try:
import sage.config
value = getattr(sage.config, key, None)
except ImportError:
passAlso, if we want to get rid of |
How do you manage to get On macOS I get |
Good catch! Indeed, a copy of old sage_conf allowed this test to pass for me on Linux. Some much for the environment.... |
|
@tobiasdiez one has to remove stuff in Also, you seem to have forgotten to revert the Volker's revert, duh...
|
|
@tobiasdiez - I've posted a rebase and a fix on tobiasdiez#17 - please verify it's OK |
|
cause it's a rebase, I'm hesitant to push it directly |
|
with my PR, docs don't build: there is also crud related to |
|
at least tests appear to work, including the one in src/sage/env.py (which I restored in my PR) |
That SAGE_ROOT is None is the expected output. Meson doesn't embed SAGE_ROOT in The problem is that Based on this, I see the following options:
Do you have a preference, or another suggestion on how to fix this? The problem with SAGE_LOCAL is similar. |
A complete removal of sage_conf is done in #40327. Shall we close this PR here and continue talking there? |
|
I don't mind to move over to #40327 |
|
@orlitzky Do you have any suggestion on how to solve the issue #40882 (comment) (since you mentioned your struggles with this test in #40869 (comment)) |
|
At the risk of becoming the "delete the test" guy, I think we should... delete the test. The purpose of the test is to ensure that But |
|
Thanks for your assessment. I've now deleted the test in #40327. Let's continue there. |
|
You are misrepresenting the test. First and foremost, it tests that you can import |
I must be missing something. What's broken, other than the test? If we really can't import sage.all without import os
old_env = {}
for k in os.environ:
if k.startswith("SAGE_"):
# Save SAGE_FOO variables
old_env[k] = os.environ[k]
os.environ[k] = ""
try:
import sage.all
finally:
# dict merge; restore SAGE_FOO
os.environ = os.environ | old_envIt looks a lot nicer in pytest without all the shenanigans, and without the part that compares meaningless variables. |
What error did you encounter with this PR when importing I would be very surprised if this got broken, especially since CI runs pytest (outside of a sage env) and pytest loads Lines 367 to 368 in f4adc25 SAGE_ env variables.
You also reported above that the sage.all import succeeded: #40882 (comment) |
It tries to import |
Minimal version of #40327 to fix the issue mentioned in https://groups.google.com/g/sage-release/c/7LhB72fmw6k/m/QINoQj8YAgAJ, as well as strengthen the CI-inclusion pattern to hopefully catch similar issues earlier next time.
📝 Checklist
⌛ Dependencies