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

Add check for build from source #2919

Merged
merged 6 commits into from
Jul 21, 2017

Conversation

baughj
Copy link
Contributor

@baughj baughj commented Jul 20, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

This is a simple change to add a warning to brew doctor if the unsupported environment variable HOMEBREW_BUILD_FROM_SOURCE is set. I believe they would be beneficial for older brew users (myself included) that have this set (I wasn't aware it was unsupported until recently).

@baughj
Copy link
Contributor Author

baughj commented Jul 20, 2017

I notice that docs/FAQ.md also needs to be updated to make a note that this flag is unsupported. I am happy to add that to this PR, or open a new one.

@chdiza
Copy link
Contributor

chdiza commented Jul 20, 2017

HOMEBREW_BUILD_FROM_SOURCE is unsupported? When did this happen?

@ilovezfs
Copy link
Contributor

b745546

@baughj
Copy link
Contributor Author

baughj commented Jul 20, 2017

It has been unsupported for a while (indeed, years). I wasn't aware of this myself until I opened an issue, so I am trying to ensure that other people don't go down a rat hole trying to diagnose something when it's an unsupported path to begin with.

@chdiza
Copy link
Contributor

chdiza commented Jul 20, 2017

It doesn't say there that it's unsupported though. It just says it's intended for homebrew developers.

I don't see the benefit of this. brew --env will already show someone whether it's set.

@baughj
Copy link
Contributor Author

baughj commented Jul 20, 2017

It also says Please do not file issues if you encounter errors when using this environment variable which, to me, is the definition of unsupported.

If a user is going to be told to not waste time opening a ticket if they encounter errors as a result of setting it - my feeling is that brew doctor, the advertised first step when opening an issue, should tell the user this, so that they don't waste their time and the time of others.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @baughj. This is something we should have done a while ago and your PR helpfully removes the item from my TODO list I added today to add it!

@@ -89,16 +89,11 @@ following conditions:
will use a bottled version of the formula, but
`brew install <formula> --enable-bar` will trigger a source build.
* The `--build-from-source` option is invoked.
* The environment variable `HOMEBREW_BUILD_FROM_SOURCE` is set.
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this line as-is because it's factually correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about adding something that this is not intended for general use? Like (for developers only).

* The machine is not running a supported version of macOS as all
bottled builds are generated only for supported macOS versions.
* Homebrew is installed to a prefix other than the standard
`/usr/local` (although some bottles support this).

In order to completely disable bottled builds, simply add a value for
the environment variable `HOMEBREW_BUILD_FROM_SOURCE` to
your profile.
Copy link
Member

Choose a reason for hiding this comment

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

👍 on deleting this.

@@ -98,6 +98,17 @@ def check_for_installed_developer_tools
EOS
end

def check_build_from_source
return unless ENV.key?("HOMEBREW_BUILD_FROM_SOURCE")
Copy link
Member

Choose a reason for hiding this comment

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

return unless ENV["HOMEBREW_BUILD_FROM_SOURCE"] is our usual style here.

@baughj
Copy link
Contributor Author

baughj commented Jul 20, 2017

OK, changed as requested and updated language to (intended for developers only) which matches the man page.

@MikeMcQuaid MikeMcQuaid merged commit e0560ff into Homebrew:master Jul 21, 2017
@MikeMcQuaid
Copy link
Member

Thanks for your first contribution to Homebrew/brew, @baughj! Without people like you submitting PRs we couldn't run this project. You rock!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants