Opened 5 years ago

Closed 5 years ago

#24014 closed enhancement (fixed)

Replace $PIP_INSTALL with sdh_pip_install helper function

Reported by: Erik Bray Owned by:
Priority: major Milestone: sage-8.1
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: c69dfda (Commits, GitHub, GitLab) Commit: c69dfdaf867b10560b91a740885e22b4ffcaeb88
Dependencies: Stopgaps:

Status badges

Description

This removes the $PIP_INSTALL environment variable in favor of a new sdh_pip_install helper function in sage-dist-helpers. This is consistent with the purpose of the library of build helper functions, and is hopefully the last time such a mass change should be made.

(I realize in https://trac.sagemath.org/ticket/21441#comment:36 I argued against making more mass updates to replace PIP_INSTALL, but I'm not wild about the environment variable either, and now that the helper functions exist this seems like the best approach for consistency's sake.)

The only change this makes to the spkg-install scripts is a bulk sed.

Change History (18)

comment:1 Changed 5 years ago by Erik Bray

Status: newneeds_review

comment:2 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Conflicts at least with #23983 and #21083.

comment:3 Changed 5 years ago by Jeroen Demeyer

I think the easiest solution would be to keep support for $PIP_INSTALL for now and simply revert the changes to dot2tex and brial on this ticket.

comment:4 Changed 5 years ago by Erik Bray

This isn't really very high priority so it can wait until those tickets are merged and then do as you said.

comment:5 Changed 5 years ago by git

Commit: 4b87c1084d610b82d8f93b89133687a8c3c2ace056ad2411c2678f205ecf062be87d368eed562a88

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

56ad241Add some docs for this function

comment:6 in reply to:  4 Changed 5 years ago by Jeroen Demeyer

Replying to embray:

This isn't really very high priority so it can wait until those tickets are merged and then do as you said.

The problem with waiting is that there could be new conflicting tickets popping up. But I will leave it to you to decide what to do.

comment:7 Changed 5 years ago by Erik Bray

Oh I see what you're saying now--keep PIP_INSTALL for now and still use it in brial and dot2tex. Not exactly a happy thing but it makes sense.

The question is: Should I open a separate ticket for later removal of PIP_INSTALL entirely? I think yes...

comment:8 Changed 5 years ago by Erik Bray

Note: This ticket did not change brial because it wasn't using pip to install the python package yet...

comment:9 Changed 5 years ago by git

Commit: 56ad2411c2678f205ecf062be87d368eed562a88e5bebd1987d82685570a15efe79944a63b5bba4a

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

e5bebd1Keep PIP_INSTALL for now but quietly mark it as deprecated.

comment:10 Changed 5 years ago by Erik Bray

Status: needs_workneeds_review

Okay, put PIP_INSTALL back for now. See #24018

comment:11 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

In appnope, you removed the error check:

-    $PIP_INSTALL . || exit $?
+    sdh_pip_install .

Given that you did not add such a check to sdh_pip_install (it would make sense to do that though), this is wrong.

comment:12 Changed 5 years ago by Erik Bray

Hmm, yes--sage-pip-install already has reasonable error messages, but in the context of the spkg-install script we still need to handle errors in it. Notably, appnope was the only package that had this error check at all (but in most cases it's the only command run so I guess it wasn't necessary).

comment:13 Changed 5 years ago by git

Commit: e5bebd1987d82685570a15efe79944a63b5bba4ac69dfdaf867b10560b91a740885e22b4ffcaeb88

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

c69dfdaAdd error handling and SAGE_SUDO support for sdh_pip_install

comment:14 Changed 5 years ago by Erik Bray

Status: needs_workneeds_review

comment:15 Changed 5 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer

Diff looks good, I'm testing a build from scratch now.

comment:16 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

comment:17 Changed 5 years ago by Erik Bray

Thanks!

comment:18 Changed 5 years ago by Volker Braun

Branch: u/embray/build/sdh-pip-installc69dfdaf867b10560b91a740885e22b4ffcaeb88
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.