Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace $PIP_INSTALL with sdh_pip_install helper function #24014

Closed
embray opened this issue Oct 11, 2017 · 19 comments
Closed

Replace $PIP_INSTALL with sdh_pip_install helper function #24014

embray opened this issue Oct 11, 2017 · 19 comments

Comments

@embray
Copy link
Contributor

embray commented Oct 11, 2017

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 #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.

Component: build

Author: Erik Bray

Branch/Commit: c69dfda

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24014

@embray embray added this to the sage-8.1 milestone Oct 11, 2017
@jdemeyer
Copy link

comment:2

Conflicts at least with #23983 and #21083.

@jdemeyer
Copy link

comment:3

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.

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:4

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

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

56ad241Add some docs for this function

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

Changed commit from 4b87c10 to 56ad241

@jdemeyer
Copy link

comment:6

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.

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:7

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...

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:8

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

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

e5bebd1Keep PIP_INSTALL for now but quietly mark it as deprecated.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

Changed commit from 56ad241 to e5bebd1

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:10

Okay, put PIP_INSTALL back for now. See #24018

@jdemeyer
Copy link

comment:11

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.

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:12

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

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

c69dfdaAdd error handling and SAGE_SUDO support for sdh_pip_install

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2017

Changed commit from e5bebd1 to c69dfda

@jdemeyer
Copy link

comment:15

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

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@embray
Copy link
Contributor Author

embray commented Oct 12, 2017

comment:17

Thanks!

@vbraun
Copy link
Member

vbraun commented Oct 20, 2017

Changed branch from u/embray/build/sdh-pip-install to c69dfda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants