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

Allow passing hash to system #2883

Merged
merged 5 commits into from Jul 18, 2017

Conversation

Projects
None yet
3 participants
@mistydemeo
Copy link
Contributor

mistydemeo commented Jul 10, 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 successfully run brew tests with your changes locally?

This adds support for passing an env hash to Formula#system and Homebrew.system. This syntax is supported by the language methods we're shadowing. The env hash is combined with the system env, so it allows passing extra environment variables or overriding environment variables in individual system/exec calls.

I missed this in #2877, which makes use of this syntax.

@mistydemeo mistydemeo requested review from MikeMcQuaid and reitermarkus Jul 10, 2017

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jul 10, 2017

I'm not thrilled with allowing this in formulae.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 10, 2017

I'm not thrilled with allowing this in formulae.

Agreed. I'd rather we continued to use ENV[] as that's the pattern we've used in formula for a long time.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 10, 2017

That's fair. My thought was that, since we're shadowing the name of a core function, it would make sense to support more of its functionality. As well, this saves us a bit of work, namely:

def make(*args)
  system({ "MAKE" => make_name }, "make", *args)
end

vs

def make(*args)
  begin
    old_make = ENV.delete "MAKE"
    ENV["MAKE"] = make_name
    system "make", *args
  rescue
    ENV["MAKE"] = old_make
  end
end

Since the pattern already provided in the builtin Ruby function handles this logic already, it avoids us having to write that logic in the places we need to do this (and avoids potential race conditions if we had multithreaded code).

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 10, 2017

I'm not thrilled with allowing this in formulae.

By the way, for reference, the method I'm aiming to use this fix is in FileUtils. This is only indirectly being called in formulae; the methods included from FileUtils see Formula#system instead of Homebrew.system or Kernel.system.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 11, 2017

I'd be OK with providing this functionality for use not in formulae (i.e. this should specifically not work in formulae.

old_make = ENV.delete "MAKE"

We use this pattern enough in the codebase already that a env "MAKE" => make_name do refactoring for both formulae and internals would be best, I think.

@mistydemeo mistydemeo force-pushed the mistydemeo:allow_passing_hash_to_system branch from 698cf9c to fd3c26f Jul 14, 2017

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 14, 2017

Updated to add an env helper, and reimplemented the make fix using that.

I briefly tried out putting this in ENV, but the fact that SharedEnvExtension isn't active everywhere we'd use this made it impractical.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 15, 2017

Updated to add an env helper,

Looks great 👍. There's a few places in build.rb, formula.rb that would use this nicely; I used grep "] = old_" to search for them.

and reimplemented the make fix using that.

Is that a fix for the issue in Homebrew/homebrew-core#15547? What was the cause?

@mistydemeo mistydemeo force-pushed the mistydemeo:allow_passing_hash_to_system branch from fd3c26f to 1e72cee Jul 16, 2017

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 16, 2017

Is that a fix for the issue in Homebrew/homebrew-core#15547? What was the cause?

That's a separate bug, which I've just pushed a fix for. It looks like cmake's configure script, when it's testing whether to use make or gmake, sets the MAKE environment variable. Since the make/gmake shim scripts respected the value of MAKE, it means our shim tried to xcrun something that's out of its PATH and bailed.

I fixed this by adjusting the environment variable to HOMEBREW_MAKE, so stuff like cmake won't be able to interfere. It still exports MAKE so subprocesses pick the make we want.

There's a few places in build.rb, formula.rb that would use this nicely

Updated to use this!

mistydemeo added some commits Jul 14, 2017

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 16, 2017

Renamed the helper to with_env; the old name was clashing with Formula#env.

@mistydemeo mistydemeo force-pushed the mistydemeo:allow_passing_hash_to_system branch from 1e72cee to 32b7e32 Jul 16, 2017

@@ -1,4 +1,5 @@
#!/bin/bash

export MAKE=${HOMEBREW_MAKE:-make}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 17, 2017

Member

@mistydemeo Do we need to export this here?

This comment has been minimized.

@mistydemeo

mistydemeo Jul 17, 2017

Contributor

It's probably not strictly necessary, but it was a suggestion in the previous PR to make sure that subprocesses call the correct make.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

👍 if this has been tested on at least ~5 formula using make and doesn't rebreak cmake.

@mistydemeo

This comment has been minimized.

Copy link
Contributor

mistydemeo commented Jul 18, 2017

Tested rebuilding cmake from source, along with ~5 things which require make including portable-ruby@2.4. Think we're good.

@mistydemeo mistydemeo merged commit f8300b2 into Homebrew:master Jul 18, 2017

2 of 3 checks passed

codecov/patch 16.12% of diff hit (target 65.75%)
Details
codecov/project 65.78% (+0.02%) compared to fb90154
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mistydemeo mistydemeo deleted the mistydemeo:allow_passing_hash_to_system branch Jul 18, 2017

@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.