Skip to content

use USE_STAGING_BINARIES splicing PIP_WHEEL_URL#1059

Closed
duanhongyi wants to merge 1 commit intoheroku:mainfrom
duanhongyi:main
Closed

use USE_STAGING_BINARIES splicing PIP_WHEEL_URL#1059
duanhongyi wants to merge 1 commit intoheroku:mainfrom
duanhongyi:main

Conversation

@duanhongyi
Copy link
Copy Markdown

No description provided.

@duanhongyi duanhongyi changed the title custom PIP_WHEEL_URL environment variable use USE_STAGING_BINARIES splicing PIP_WHEEL_URL Aug 27, 2020
Copy link
Copy Markdown
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you for the PR :-)

Could you describe the motivation for this change? (For example what's the use-case and is this for Heroku or something else?) Generally a PR should always have a description that describes the "why" that isn't necessarily apparent from the "what" of the diff itself. There's a good guide to commit messages here that may be of interest :-)
https://chris.beams.io/posts/git-commit/

If this is something that is accepted, it will also:

  • need some tests
  • need to use a different approach for the env var, depending on whether this is for Heroku use or not (since app env vars aren't automatically set in the build enviroment; buildpacks have to load ENV_DIR etc)

Many thanks!

@edmorley
Copy link
Copy Markdown
Member

Oops forgot to also say - if you make further changes to the PR, please add them to this one - no need to make a new PR each time :-)

@edmorley
Copy link
Copy Markdown
Member

@duanhongyi Hi! I don't suppose you saw my questions above? :-)

@duanhongyi
Copy link
Copy Markdown
Author

I'm really sorry. I just came back from my vacation.

The motivation for this RP is simple. In my opinion, in USE_STAGING_BINARIES should contain the PIP installation script.

@edmorley
Copy link
Copy Markdown
Member

The motivation for this RP is simple. In my opinion, in USE_STAGING_BINARIES should contain the PIP installation script.

@duanhongyi Hi! Thank you for the reply. However I'm still not sure what the motivation is? For example:

  • what are you trying to do with the buildpack
  • why does the current implementation make that hard
  • what are you doing to work around it in the meantime
  • how does this change make that easier?

Without more details we won't be able to accept the change.

@duanhongyi duanhongyi closed this Sep 25, 2020
@edmorley
Copy link
Copy Markdown
Member

@duanhongyi Hi! I suspect your use-case might be fixed by #1085 (which I needed to do for other reasons, but conveniently makes custom S3 URLs for the pip case easier too).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants