Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21821 closed enhancement (fixed)

Avoid Sage-specific variables in autogen/pari

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.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 jdemeyer)

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 jdemeyer

  • 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 jdemeyer

  • Dependencies #21820 deleted

comment:3 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/do_not_hardcode_path_of_pari_desc

comment:4 Changed 3 years ago by jdemeyer

  • Commit set to 0553d6b4892e82a731a50a46c445487042541bc4
  • Status changed from new to needs_review

New commits:

0553d6bDo not hardcode path of pari.desc

comment:5 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:6 Changed 3 years ago by git

  • Commit changed from 0553d6b4892e82a731a50a46c445487042541bc4 to ed1bfdd68ee1c7dd57737bb40f5172d560c8a2d5

Branch pushed to git repo; I updated commit sha1. New commits:

ed1bfddDon't use SAGE_SRC in sage_src_pari()

comment:7 Changed 3 years ago by jdemeyer

  • 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 jdemeyer

  • Status changed from needs_review to needs_work

comment:9 Changed 3 years ago by git

  • Commit changed from ed1bfdd68ee1c7dd57737bb40f5172d560c8a2d5 to c8946252439f15f6f67927074639eb2b1e98b539

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c894625Don't use SAGE_SRC in sage_src_pari()

comment:10 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Do not hardcode path of pari.desc and don't use SAGE_SRC to Avoid Sage-specific variables in autogen/pari

comment:11 Changed 3 years ago by git

  • Commit changed from c8946252439f15f6f67927074639eb2b1e98b539 to bc8a3233c86fea1cb4ee11e7a6a71a12fd9d9e77

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bc8a323Don't use SAGE_SRC in sage_src_pari()

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 3 years ago by embray

I think the pari_share() makes sense--I 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 one-off 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 jdemeyer

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 jdemeyer

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 embray

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 is--especially for sage_setup.docbuild (but then maybe that should be moved either into the sage package itself, or into its own package).

Last edited 3 years ago by embray (previous) (diff)

comment:17 follow-up: Changed 3 years ago by embray

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 jdemeyer

Maybe sage_setup is installed just for testing, such that it can be imported in doctests?

comment:19 Changed 3 years ago by jdemeyer

sage_setup was created in #16431. Already there, it was installed.

comment:20 in reply to: ↑ 17 Changed 3 years ago by jdemeyer

Replying to embray:

Put another way, I'm not sure what the advantage is to not using SAGE_SRC here

I see it as part of push towards less Sage-specific things in the Sage build system, see also #15105 and #21507 in general.

comment:21 Changed 3 years ago by embray

  • 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 vbraun

  • 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 follow-up: Changed 3 years ago by chapoton

  • 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/sage-devel/eSYPSfZOb9k

comment:24 in reply to: ↑ 23 Changed 3 years ago by dimpase

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 version-agnostic...)

Last edited 3 years ago by dimpase (previous) (diff)

comment:25 follow-up: Changed 3 years ago by defeo

The b"" syntax is supported in Python 2.7.

comment:26 in reply to: ↑ 25 Changed 3 years ago by dimpase

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 jdemeyer

I created #21874.

Note: See TracTickets for help on using tickets.