Opened 4 years ago

Closed 4 years ago

#24014 closed enhancement (fixed)

Replace $PIP_INSTALL with sdh_pip_install helper function

Reported by: embray 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 4 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Conflicts at least with #23983 and #21083.

comment:3 Changed 4 years ago by jdemeyer

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 follow-up: Changed 4 years ago by embray

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 4 years ago by git

  • Commit changed from 4b87c1084d610b82d8f93b89133687a8c3c2ace0 to 56ad2411c2678f205ecf062be87d368eed562a88

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

56ad241Add some docs for this function

comment:6 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

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 4 years ago by embray

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 4 years ago by embray

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

comment:9 Changed 4 years ago by git

  • Commit changed from 56ad2411c2678f205ecf062be87d368eed562a88 to e5bebd1987d82685570a15efe79944a63b5bba4a

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 4 years ago by embray

  • Status changed from needs_work to needs_review

Okay, put PIP_INSTALL back for now. See #24018

comment:11 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_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 4 years ago by embray

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 4 years ago by git

  • Commit changed from e5bebd1987d82685570a15efe79944a63b5bba4a to c69dfdaf867b10560b91a740885e22b4ffcaeb88

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 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:15 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

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

comment:16 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:17 Changed 4 years ago by embray

Thanks!

comment:18 Changed 4 years ago by vbraun

  • Branch changed from u/embray/build/sdh-pip-install to c69dfdaf867b10560b91a740885e22b4ffcaeb88
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.