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

superenv: port to generic OS. #457

Merged
merged 1 commit into from
Jul 12, 2016
Merged

superenv: port to generic OS. #457

merged 1 commit into from
Jul 12, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 4, 2016

  • 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
@DomT4
Copy link
Member

DomT4 commented Jul 5, 2016

I know the benefits outweigh the costs here, but all this refactoring for cross-platform compatibility makes it a complete nightmare to quickly gather a full picture in terms of what calling require on x actually drags in. On the record, I'll buy anyone who invents an Atom plugin to automatically follow requires a 🍺 or five.

@@ -13,7 +13,7 @@ def sh
ENV.activate_extensions!

if superenv?
ENV.x11 = MacOS::X11.installed?
ENV.x11_if_installed
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this was/is supposed to do? I don't really understand this, but I'm not sure the replacement is equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Set ENV.x11 if MacOS::X11.installed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this massively confusing, also when compared to what x11 means in stdenv. But oh well, I guess legacy …

Copy link
Member Author

Choose a reason for hiding this comment

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

Open to better suggestions. Have a generic_sh method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. I never used brew sh and I'm not entirely sure what that line is supposed to do (in more broader terms, I understood your explanation from above).

@UniqMartin
Copy link
Contributor

I know the benefits outweigh the costs here, but all this refactoring for cross-platform compatibility makes it a complete nightmare to quickly gather a full picture in terms of what calling require on x actually drags in.

You might want to give brew graph-require (a quick and undocumented late-night hack, including code comments that just stop mid-sentence as I just noticed 😂) a try, e.g.:

brew graph-require --start=extend/ENV/super | dot -Tpdf > super.pdf
open super.pdf

Graph for extend/ENV/super. Solid lines represent plain requires, dashed lines typically conditional requires. The heuristics for that are really simple, thus better check if in doubt.

On the record, I'll buy anyone who invents an Atom plugin to automatically follow requires a 🍺 or five.

This would involve touching JavaScript, something I'm not considering just yet. :eek:

@MikeMcQuaid MikeMcQuaid merged commit 0d189fa into Homebrew:master Jul 12, 2016
@MikeMcQuaid MikeMcQuaid deleted the superenv-port branch July 12, 2016 11:01
@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
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants