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

stdenv: port to generic OS. #456

Merged
merged 1 commit into from
Jul 12, 2016
Merged

stdenv: port to generic OS. #456

merged 1 commit into from
Jul 12, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 4, 2016 •
edited
Loading

  • 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?

More work towards making Homebrew/brew a cross-platform core for e.g. Linuxbrew and Tigerbrew too.

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 4, 2016
scpeters
scpeters reviewed Jul 5, 2016
View reviewed changes
Library/Homebrew/extend/os/mac/ENV/std.rb
end

def homebrew_extra_pkg_config_paths
["#{HOMEBREW_ENV_PATH}/pkgconfig/#{MacOS.version}"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this code is moved here, but I don't see any place where this function homebrew_extra_pkg_config_paths is invoked

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@scpeters
Copy link
Contributor

scpeters commented Jul 5, 2016

I was just peeking around and made a comment, but I'm not sure I'll be able to do a full review. Looks like nice stuff.

UniqMartin
UniqMartin reviewed Jul 6, 2016
View reviewed changes
Library/Homebrew/extend/ENV/std.rb
@@ -59,33 +52,17 @@ def setup_build_environment(formula = nil)
gcc_formula = gcc_version_formula($&)
append_path "PATH", gcc_formula.opt_bin.to_s
end
end
alias_method :setup_generic_build_environment, :setup_build_environment
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this sounds better, but wouldn't it be safer to stick to a naming scheme where these aliased methods are always prefixed with generic_?

@UniqMartin
Copy link
Contributor

A few quirks that need to be ironed out, but nothing major. I'd appreciate if there was a second round of review once the initial round of comments has been addressed (and once I had time to review the related superenv changes and cross check with them).

How was this PR tested locally? I'd suggest to build some formulae of varying complexity from source with --env=std (cover multiple build systems, use formulae that exercise some of the relocated features from ENV) and to also run their tests with brew test (as that further exercises Stdenv).

UniqMartin
UniqMartin reviewed Jul 12, 2016
View reviewed changes
Library/Homebrew/extend/ENV/std.rb
@@ -59,33 +52,18 @@ def setup_build_environment(formula = nil)
gcc_formula = gcc_version_formula($&)
append_path "PATH", gcc_formula.opt_bin.to_s
end
end
alias generic_setup_build_environment setup_build_environment
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this and all other alias instances to use alias_method instead, as we've done in the past and is now codified in our RuboCop configuration as of #489?

Copy link
Member Author

Choose a reason for hiding this comment

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

🆗, didn't see our Rubocop change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I only merged it a few moments ago, but you 👍ed it. 😉

@UniqMartin
Copy link
Contributor

Should be fine except for the test failure and the final style nit (that didn't really apply earlier).

@MikeMcQuaid MikeMcQuaid merged commit 498e81c into Homebrew:master Jul 12, 2016
@MikeMcQuaid MikeMcQuaid deleted the stdenv-generic branch July 12, 2016 10:39
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 12, 2016
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
@MikeMcQuaid @souvik1997
stdenv: port to generic OS. (Homebrew#456)
1f048d7
iMichka pushed a commit to iMichka/brew that referenced this pull request Sep 11, 2017
@sjackman
Dockerfile: Move the repo to ~/.linuxbrew/Homebrew (Homebrew#456)
4eb316a
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MikeMcQuaid @scpeters @UniqMartin @BrewTestBot