#21821 closed enhancement (fixed)
Avoid Sagespecific variables in autogen/pari
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  interfaces  Keywords:  
Cc:  defeo  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  bc8a323 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
In src/sage_setup/autogen/pari
: do not hardcode the path of pari.desc
and don't use SAGE_SRC
.
Change History (27)
comment:1 Changed 3 years ago by
 Summary changed from Do not hardcode location to pari.desc to Do not hardcode path of pari.desc
comment:2 Changed 3 years ago by
 Dependencies #21820 deleted
comment:3 Changed 3 years ago by
 Branch set to u/jdemeyer/do_not_hardcode_path_of_pari_desc
comment:4 Changed 3 years ago by
 Commit set to 0553d6b4892e82a731a50a46c445487042541bc4
 Status changed from new to needs_review
comment:5 Changed 3 years ago by
comment:6 Changed 3 years ago by
 Commit changed from 0553d6b4892e82a731a50a46c445487042541bc4 to ed1bfdd68ee1c7dd57737bb40f5172d560c8a2d5
Branch pushed to git repo; I updated commit sha1. New commits:
ed1bfdd  Don't use SAGE_SRC in sage_src_pari()

comment:7 Changed 3 years ago by
 Summary changed from Do not hardcode path of pari.desc to Do not hardcode path of pari.desc and don't use SAGE_SRC
comment:8 Changed 3 years ago by
 Status changed from needs_review to needs_work
comment:9 Changed 3 years ago by
 Commit changed from ed1bfdd68ee1c7dd57737bb40f5172d560c8a2d5 to c8946252439f15f6f67927074639eb2b1e98b539
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c894625  Don't use SAGE_SRC in sage_src_pari()

comment:10 Changed 3 years ago by
 Description modified (diff)
 Summary changed from Do not hardcode path of pari.desc and don't use SAGE_SRC to Avoid Sagespecific variables in autogen/pari
comment:11 Changed 3 years ago by
 Commit changed from c8946252439f15f6f67927074639eb2b1e98b539 to bc8a3233c86fea1cb4ee11e7a6a71a12fd9d9e77
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bc8a323  Don't use SAGE_SRC in sage_src_pari()

comment:12 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:13 followup: ↓ 15 Changed 3 years ago by
I think the pari_share()
makes senseI like that it uses whatever the appropriate gp
is to get the right path. Though I think this would make sense to have somewhere in sage itself, and be stored as a variable in sage.env
, not just as a oneoff in this code.
I don't think the change to sage_src_pari()
makes sense because it makes the assumption that the code is being run directly from the sage source tree. Of course, that's the typical case. But the code is not otherwise designed in such a way that that must be the case, nor do I think it necessarily should be. I like the way it was done better.
comment:14 Changed 3 years ago by
So, what is your solution for sage_src_pari()
which would be compatible with #21613?
comment:15 in reply to: ↑ 13 Changed 3 years ago by
Replying to embray:
I don't think the change to
sage_src_pari()
makes sense because it makes the assumption that the code is being run directly from the sage source tree.
Let me clarify: that code is only meant to be run during the build of Sage. And I do think it is a reasonable assumption that the current working directory at build time is the source directory (*). Most setup.py
files make that assumption, so if you see this sage_setup
stuff as an extension of setup.py
, it makes perfect sense.
(*) To be very precise, let's define the source directory as the directory containing setup.py
, which is $SAGE_SRC = $SAGE_ROOT/src
in Sage.
comment:16 Changed 3 years ago by
Hmm, I agree with you, but what bothers me then is that sage_setup
is installed at all, if it's only meant as a supplement to setup.py
(and that being the case, we could also vastly simplify the setup.py
by moving things like distutils commands into sage_setup
as well, which wouldn't be a bad idea anyways).
To follow up, I feel like if sage_setup
can be installed, then it should also in principle work outside a build context. I think it would be better if it simply weren't installed at all, but I think there are reasons it isespecially for sage_setup.docbuild
(but then maybe that should be moved either into the sage
package itself, or into its own package).
comment:17 followup: ↓ 20 Changed 3 years ago by
Put another way, I'm not sure what the advantage is to not using SAGE_SRC
here unless everything in sage_setup
is consistently designed to only be used at build time.
comment:18 Changed 3 years ago by
Maybe sage_setup
is installed just for testing, such that it can be imported in doctests?
comment:19 Changed 3 years ago by
sage_setup
was created in #16431. Already there, it was installed.
comment:20 in reply to: ↑ 17 Changed 3 years ago by
comment:21 Changed 3 years ago by
 Reviewers set to Erik Bray
 Status changed from needs_review to positive_review
Ok, I think I agree in general. I don't think sage_setup
should be installed at all, at least not by default. I'll open a new issue for that.
comment:22 Changed 3 years ago by
 Branch changed from u/jdemeyer/do_not_hardcode_path_of_pari_desc to bc8a3233c86fea1cb4ee11e7a6a71a12fd9d9e77
 Resolution set to fixed
 Status changed from positive_review to closed
comment:23 followup: ↓ 24 Changed 3 years ago by
 Commit bc8a3233c86fea1cb4ee11e7a6a71a12fd9d9e77 deleted
The following line that was added here is apparently not compatible with python 3:
out = gp.communicate("print(default(datadir))")[0]
See https://groups.google.com/forum/#!topic/sagedevel/eSYPSfZOb9k
comment:24 in reply to: ↑ 23 Changed 3 years ago by
Replying to chapoton:
yes, it should be bytes, not string, i.e. try this:
out = gp.communicate(b'print(default(datadir))')[0]
(don't know how to make this versionagnostic...)
comment:25 followup: ↓ 26 Changed 3 years ago by
The b""
syntax is supported in Python 2.7.
comment:26 in reply to: ↑ 25 Changed 3 years ago by
Replying to defeo:
The
b""
syntax is supported in Python 2.7.
Ah, OK, it has no effect in Python2. Yes, this would be the good fix then.
comment:27 Changed 3 years ago by
I created #21874.
New commits:
Do not hardcode path of pari.desc