Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Support installing tools via Homebrew (take 2) #474

Merged
merged 10 commits into from
Nov 2, 2020
Merged

Support installing tools via Homebrew (take 2) #474

merged 10 commits into from
Nov 2, 2020

Conversation

calavera
Copy link
Contributor

@mraerino opened a PR to support homebrew long time ago (#411). We discovered that some people had Brewfile files in their sites and this integration broke their builds because we were trying to install packages that were not necessary.

Supporting homebrew is a great idea, and I'd like to bring this feature back. In order to not break builds again, I renamed the Brewfile to Brewfile.netlify. I'd expect no breaking changes since I just made this up.

@calavera calavera added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 28, 2020
@calavera calavera requested a review from mraerino September 28, 2020 20:51
@calavera calavera requested a review from a team as a code owner September 28, 2020 20:51
@mraerino
Copy link
Contributor

can we hide this whole process behind some environment variable?
that way it's probably safer for people working on the build process

mraerino
mraerino reviewed Sep 28, 2020
View reviewed changes
run-build-functions.sh Outdated
if [ -f Brewfile.netlify ] || [ ! -z "$HOMEBREW_BUNDLE_FILE" ]
then
echo "Installing Homebrew dependencies from ${HOMEBREW_BUNDLE_FILE:-Brewfile.netlify}"
brew bundle --file Brewfile.netlify
Copy link
Contributor

@mraerino mraerino Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

i'm not sure what happens here if you set HOMEBREW_BUNDLE_FILE to something custom.

what do you think about making Brewfile.netlify a default value for HOMEBREW_BUNDLE_FILE inside the if and remove the --file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah.

mraerino and others added 7 commits September 28, 2020 14:36
@mraerino @calavera
Support installing tools via homebrew
96a3c6f
@mraerino @calavera
Move brew install before all other dependencies
031d697 
Mainly for allowing native node modules to use homebrew stuff
@mraerino @calavera
Log when installing Homebrew dependencies
2706fac
@mraerino @calavera
Warn about status of Homebrew support
c588d67
@calavera
Rename Brewfile to Brewfile.netlify
75fc194 
That way we can test this experiment without breaking
builds for people that already have Brewfile in their
sites.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Fix file name in includeD_software.md.
b7c7cfb 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Set custom file as default env variable.
55d0b61 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Temporarily remove wasmer.
92f69f9 
To see if that's the real issue crashing CI.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Remove wasmer permanently.
19b59c0 
According to our logs, nobody has ever used it.

Signed-off-by: David Calavera <david.calavera@gmail.com>
This was referenced Oct 3, 2020
Merged
Closed
@depadiernos
Copy link

Is there anything needed here before we can review and merge?

@ehmicky ehmicky requested a review from mraerino October 30, 2020 14:35
mraerino
mraerino reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

i'm not too sure about removing wasmer in this PR.
I was not able to find any evidence of anyone using it in the last 7 days directly, but i'm unsure.

I'd defer this decision to the team

@cecton
Copy link
Contributor

cecton commented Oct 30, 2020

This PR is blocking me personally for a month now (other people for longer). I think if people really want WASMER you either need to upgrade your ubuntu image or contact the people from WASMER and ask them to compile with an older version of GLIBC.

@calavera
Copy link
Contributor Author

i'm not too sure about removing wasmer in this PR.

We need to do it anyways, and this is the last PR that actually passes CI because wasmer doesn't work anymore, so I'm all for merging both and move on. It really have never been used.

mraerino
mraerino approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

cool with me.

@syrusakbary
Copy link

We need to do it anyways, and this is the last PR that actually passes CI because wasmer doesn't work anymore, so I'm all for merging both and move on. It really have never been used.

Update: we have actually fixed Wasmer in master, it no longer have the glibc issue thanks to this PR:

wasmerio/wasmer#1755

PS: if you need I can update the binary manually so it's also fixed when you install Wasmer 1.0.0-alpha4 (the latest published version).

I'd recommend you to keep Wasmer, super cool things are coming in the next weeks!

@calavera
Copy link
Contributor Author

I'd recommend you to keep Wasmer, super cool things are coming in the next weeks!

people will be able to install wasmer when needed via homebrew, and update to newer versions when they want instead of depending on us to keep up with new releases.

@syrusakbary
Copy link

syrusakbary commented Oct 30, 2020 •
edited
Loading

people will be able to install wasmer when needed via homebrew

Yeah, that's a great alternative.

Although being able to cache wasmer artifacts independently in-between builds will make a bit of a difference in the future!

@ingride ingride self-assigned this Oct 30, 2020
@cecton
Copy link
Contributor

cecton commented Oct 31, 2020

people will be able to install wasmer when needed via homebrew, and update to newer versions when they want instead of depending on us to keep up with new releases.

I made #477 because I'm wasting a lot of build time downloading Rust at every build. Does installing via homebrew would solve that?

@ingride
Copy link
Contributor

ingride commented Nov 2, 2020 •
edited
Loading

@cecton thanks for adding the Rust PR, we can look into supporting that as part of our build image, but meanwhile, as soon as this PR is released, you'd be able to install Rust via homebrew which should help.

ingride
ingride approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@ingride ingride left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for making the change

@ingride ingride merged commit 4b48fbd into xenial Nov 2, 2020
@ingride ingride deleted the feat/homebrew branch November 2, 2020 13:16
@depadiernos depadiernos mentioned this pull request Nov 2, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mraerino mraerino mraerino approved these changes

@ingride ingride ingride approved these changes

Assignees

@ingride ingride

Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@calavera @mraerino @depadiernos @cecton @syrusakbary @ingride