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

[email protected]: do not symlink as pip3 and wheel3 #62949

Closed
wants to merge 1 commit into from

Conversation

fxcoudert
Copy link
Member

Fixes #62911

@BrewTestBot BrewTestBot added the legacy Relates to a versioned @ formula label Oct 15, 2020
@SMillerDev
Copy link
Member

I think this warrants a revbump

@fxcoudert
Copy link
Member Author

fxcoudert commented Oct 15, 2020

Revision-bumping sadly will not remove the previously created symlinks, will it? (since they're created in postinstall)
But brew cleanup should do it, I think.

@SMillerDev
Copy link
Member

I'm not sure about that, but at least it'll make sure everyone gets the fix. Which was what I was more worried about.

@iMichka
Copy link
Member

iMichka commented Oct 15, 2020

Maybe if you revbump [email protected] to override these symlinks?

Just a note: last time we made a Python migration, we were bitten by a similar issue if I remember well. But it's hard to test upfront to avoid this mistake.

@SMillerDev
Copy link
Member

Maybe we could make an audit/rubocop to make sure only one formula has these? @MikeMcQuaid probably has a better idea on how feasible that would be.

@MikeMcQuaid
Copy link
Member

Maybe if you revbump [email protected] to override these symlinks?

Yes, this could work.

Maybe we could make an audit/rubocop to make sure only one formula has these? @MikeMcQuaid probably has a better idea on how feasible that would be.

Specifying "only one formula" probably isn't feasible. Having brew audit check "no python formula except PYTHON_ALLOWED_FORMULA has these symlinks" seems sensible.

Something like https://github.com/Homebrew/brew/blob/7f422af7897f4f58cc84079ccb6f1d25ac66a759/Library/Homebrew/dev-cmd/audit.rb#L985-L994 will allow checking this only if the formula is installed. I'd suggest this be done before this PR is merged.

@fxcoudert
Copy link
Member Author

fxcoudert commented Oct 17, 2020

I'd suggest this be done before this PR is merged

The original reporter's use case (CI testing) would benefit from merging this as-is, and looking at systematic improvements later. Without revision bump, it will only affect new users and CI systems, which is still important.

@SMillerDev
Copy link
Member

It would only affect those that build from source right? Or do you plan on overwriting the bottles?

@fxcoudert
Copy link
Member Author

My plan was overwriting the bottles: isn't that what the merging bot does, when there is no revision?

@SMillerDev
Copy link
Member

I don't think it does.

@fxcoudert
Copy link
Member Author

@SMillerDev see for example this PR: #58881
It was auto-merged, does not have a revision bump, and lead to two commits: 4f17008 and 731f2b1

@BrewTestBot
Copy link
Member

:shipit: @fxcoudert has triggered a merge.

@MikeMcQuaid
Copy link
Member

The original reporter's use case (CI testing) would benefit from merging this as-is, and looking at systematic improvements later. Without revision bump, it will only affect new users and CI systems, which is still important.

My issue is: here we are several days later and no-one has opened an issue or a PR to track this work. We've got to get better at adding regression tests like this or things get forgotten and will repeatedly reoccur in future. This work should block just getting a fix out there and is definitely more important than e.g. migrating more formulae.

@fxcoudert
Copy link
Member Author

here we are several days later

Testing finished about 24 hours ago, and merging was yesterday.

no-one has opened an issue or a PR to track this work

I have not forgotten about it, even though I have not filed an issue, this tab is open in my browser because I know it's in my to-do list. All help is welcome, especially to write an audit, as I'm totally ignorant of the brew codebase.

Also, there already is an open issue, which was purposely not closed, at #62911

@fxcoudert
Copy link
Member Author

And now there is a tracking issue for the brew side: Homebrew/brew#8949

@MikeMcQuaid
Copy link
Member

@fxcoudert Thanks for opening the tracking issue 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy Relates to a versioned @ formula
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installation of Python 3.8 and Python 3.9 side by side breaks pip version.
5 participants