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.

Sign up for GitHub

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

Jump to bottom

homebrew_bootsnap: require some developer tools. #10694

Merged
merged 1 commit into from Feb 25, 2021
Merged

homebrew_bootsnap: require some developer tools. #10694

merged 1 commit into from Feb 25, 2021

Conversation

MikeMcQuaid
Copy link
Member

Do a very basic developer tools check to ensure that we can compile things. We cannot use DevelopmentTools.installed? because this has much higher speed requirements and needs to be run before we require anything else.

CC @Bo98 who mentioned this

@MikeMcQuaid
homebrew_bootsnap: require some developer tools.
634efba 
Do a very basic developer tools check to ensure that we can compile
things. We cannot use `DevelopmentTools.installed?` because this has
much higher speed requirements and needs to be run before we `require`
anything else.
@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Feb 24, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@MikeMcQuaid MikeMcQuaid mentioned this pull request Feb 24, 2021
Merged
@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021 •
edited

Looks good. I actually think the check should be only on the LoadError case. If they previously installed bootsnap correctly, then we shouldn't care if the dev tools later disappear, like on a Big Sur update. (I'm not sure how we handle bootsnap updates though - I think we just aren't.)

I think there's some edge cases (like outdated CLT versions) that could go pass this - would it make any sense to make brew doctor invokations not install any bundler gems, so that easy debugging can be performed? I know I'm complicating things - sorry!

@MikeMcQuaid
Copy link
Member Author

I actually think the check should be only on the LoadError case.

Makes sense to me 👍🏻

I think there's some edge cases (like outdated CLT versions) that could go pass this - would it make any sense to make brew doctor invokations not install any bundler gems, so that easy debugging can be performed? I know I'm complicating things - sorry!

I'd rather not special case commands but the failure case should probably be made more graceful.

@Bo98
Copy link
Member

Bo98 commented Feb 24, 2021

the failure case should probably be made more graceful.

Yeah, that would make sense.

@Bo98
Copy link
Member

Bo98 commented Feb 25, 2021

I'll merge this version now since there were a couple of reported problems from people with no dev tools.

@Bo98 Bo98 merged commit 74f7d2f into Homebrew:master Feb 25, 2021
@MikeMcQuaid MikeMcQuaid deleted the bootsnap_dev_tools branch February 25, 2021 11:02
@MikeMcQuaid
Copy link
Member Author

Thanks for merging @Bo98.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 28, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BrewTestBot BrewTestBot BrewTestBot approved these changes

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MikeMcQuaid @BrewTestBot @Bo98