src/sage/features/sagemath.py: Add feature SAGE_SRC #37650
Conversation
|
That's an interesting solution, using features more is a good idea. Here It is cool that it addresses the failures in |
Yes, this feature test does take care of this situation, but at some point we will probably also want to get rid of the SAGE_SRC fallback to SAGE_LIB. |
|
That'd be great. |
mkoeppe
force-pushed
the
feature_SAGE_SRC
branch
2 times, most recently
from
6f2537c
to
0aafe3f
Compare
|
Setting to "positive review" based on @kiwifb's review. |
| @@ -111,6 +111,7 @@ def read_distribution(src_file): | |||
|
|
|||
| EXAMPLES:: | |||
|
|
|||
| sage: # needs SAGE_SRC | |||
| sage: from sage.env import SAGE_SRC | |||
| sage: from sage.misc.package_dir import read_distribution | |||
| sage: read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'tdlib.pyx')) | |||
There was a problem hiding this comment.
This line # needs tdlib as explained in #37645. You act as if SAGE_SRC indicates that you have the whole source tree, but in fact sagemath-standard sdist contains VERSION.txt afaict.
There was a problem hiding this comment.
A fair point that I had not thought about. I was only thinking of the case where you test with an installation of sage that is not in source. In which case VERSION.txt is not installed and cannot be found.
There was a problem hiding this comment.
in fact
sagemath-standardsdist containsVERSION.txtafaict.
Very good point. And in fact all of the modularized distributions have VERSION.txt too.
Suggestions what we should test?
There was a problem hiding this comment.
Presence of the bin or doc folders? It also has to be something that will be stable - for some time at least.
There was a problem hiding this comment.
Producing a sdist right now to make sure I do not say something stupid.
There was a problem hiding this comment.
Why not add a sentinel file in src/sage/features/ that is never included in any distribution? Using an arbitrary existing file is just asking for trouble when later that file is changed for an unrelated reason, etc.
OTOH, it feels weird that testing sage-setup needs SAGE_SRC. The read_distribution() function seems like a part of sage-setup not of sagelib, and why can't it be unit tested instead of testing with a .pyx from the sage source?
There was a problem hiding this comment.
I totally get what you mean about the test, but it is meant to test "distribution", so stuff like sagemath-bliss or as in this case sagemath-tdlib. If you want to keep things in source tree, you have to provide the test suite with a "fake" mini distribution to test.
As for the file to test, it is arbitrary and I would have probably chosen pyproject.tml.m4 myself. But I think the file chosen is there for a long time which is a criterion.
There was a problem hiding this comment.
What is wrong with a file src/sage/features/this-file-only-in-the-source.txt which IIUC it's never included in a sdist by default (https://setuptools.pypa.io/en/latest/userguide/miscellaneous.html)
The file can have a simple explanation of its purpose, being a text file.
There was a problem hiding this comment.
Using an arbitrary existing file is just asking for trouble when later that file is changed for an unrelated reason, etc.
Well, this particular file I chose is not arbitrary but by its design has exactly the right properties.
There was a problem hiding this comment.
The
read_distribution()function seems like a part ofsage-setupnot of sagelib
No, it's in the right place. This module, like sage.features, is part of sagemath-environment.
|
Documentation preview for this PR (built with commit 576d8bf; changes) is ready! 🎉 |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Let's get this in please |
mkoeppe
force-pushed
the
feature_SAGE_SRC
branch
from
9a761f7
to
576d8bf
Compare
|
I was reminded of this because of #37645 which address only the |
|
I'm not arguing that my comment was not offtopic. It was. But there's no record of who marked it as offtopic (it wasn't me); I don't like that someone can anonymously hide comments. FWIW, I do NOT have this ability. It seems can mark my own comments as offtopic, but not others (not even in my own PRs). It's the same asymmetry where some developers can modify PRs they don't own, but others don't have this ability (I don't). |
|
Thanks, Francois. |
This feature is for conditionalizing some doctests.
Fixes doctest failure reported in #37645.
📝 Checklist
⌛ Dependencies