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

Make the scripts that aren't in public/ error out with a message #104

Merged
merged 5 commits into from
Mar 7, 2021

Conversation

pradyunsg
Copy link
Member

Toward #61.

/cc @pypa/pip-committers, since I'd like to run this past other folks' eyes as well (and get a green tick or two) because this is changing something that'll affect a lot of users. I don't want to do something that can be this disruptive unilaterally.

@pradyunsg
Copy link
Member Author

@pfmoore @uranusjr nudge.

@pfmoore
Copy link
Member

pfmoore commented Mar 7, 2021

Sorry, TBH I don't have time to do much more than a very superficial look at the moment. And I've not really been following what's going on with the moves in this repo. But from a quick skim, this looks OK.

@pradyunsg
Copy link
Member Author

No worries, I'll quickly summarise what's up with this repository:

I rewrote the generation pipeline in #85, to be Python 3 based, and to not depend on invoke. It's also pretty now (because it uses rich). That was most of the code-related commotion in this repository. I also triaged the entire issue tracker, and trimmed any open issues / PRs that were resolved or addressed. All of that stuff is unrelated to this PR. :)


In #61, there's discussion about what we should do to make it easier to make a pip release that drops Python minor version support (eg: Python 3.5).

The main part of that plan is to change how we serve scripts from this repository. Now, we basically serve only the public/ folder in this repository, and those would go under bootstrap.pypa.io/pip/.

The important thing here is that it changes what url we serve our existing scripts on, from bootstrap.pypa.io/2.7/get-pip.py to bootstrap.pypa.io/pip/2.7/get-pip.py.

In exchange, it simplifies the server-side deployment setup and allows us to add additional files easily (add a public/3.6/get-pip.py in this repository). The main "issue" is that we're changing the URL, which results in churn for people (read: broken pipeline).

If the overall plan (of having a public folder in this repository and change old scripts with a message about the new location) is "not horrible" and the message in this PR's moved.py is "good enough", just let me know. I think that's all I am really asking for feedback on. :)

@pfmoore
Copy link
Member

pfmoore commented Mar 7, 2021

Ah right. I missed most of the rewrites because I don't follow this repo, so I only see direct pings. Strong +1 on having the "old Python" scripts served as bootstrap.pypa.io/pip/2.7/get-pip.py (with the main get-pip.py still at the top level) - essentially the same as virtualenv does it.

The message looks fine, too. I'm sure there will be some complaints that we're renaming at all, but 🤷

@uranusjr
Copy link
Member

uranusjr commented Mar 7, 2021

I took a quick look and everything seems fine to me. Can’t say I understand much about the whole process though, so take that as you will. I have an unrelated idea while looking at the code though, will file a separate issue for that.

Edit: turns out I’m not the only one thought about it (#88), so I’ll comment there instead.

@pradyunsg
Copy link
Member Author

Let's do this.

@T2L
Copy link

T2L commented Mar 12, 2021

@pradyunsg why not provide a simple 301 redirect to a new location rather than break everyone's pipelines?

@pradyunsg
Copy link
Member Author

Because that would've broken pipelines too, and in a more subtle way with a bad error message. From #61 (comment):

We can't serve redirects from the current [url] since curl does not follow redirects by default.

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

Successfully merging this pull request may close these issues.

4 participants