GHC 2017-03-08

12 comments.

, https://git.io/vygzN in Homebrew/brew
By the way, should we also give `$HOMEBREW_PREFIX/include` the same treatment to prevent opportunistic inclusion of header only libraries? (Not remotely as harmful for sure.)

, https://git.io/vygzA in Homebrew/brew
> does that work with non-pkg-config crap too?

It should work with reasonable build systems. FFmpeg builds fine, for instance.

Not sure about crap though (e.g. build scripts hardcoded to look into `/usr/local/lib`, `/opt/local/lib`, `/sw/lib`, etc). Maybe this is a good way to uncover crappy projects.

, https://git.io/vyguF in Homebrew/brew
> It looks like it works if we change that bit to
> 
>     paths = deps.map { |d| d.opt_lib.to_s }

Yep, that's what I'm saying too.

, https://git.io/vygub in Homebrew/brew
Okay, took another look and it is indeed pkg-config that enables success location of libraries when `/usr/local/lib` is blocked. For instance, there's no problem with `x264`, because from FFmpeg's configure:

```sh
enabled libx264           && { use_pkg_config x264 "stdint.h x264.h" x264_encoder_encode ||
                               { require libx264 x264.h x264_encoder_encode -lx264 &&
                                 warn "using libx264 without pkg-config"; } } &&
                             { check_cpp_condition x264.h "X264_BUILD >= 118" ||
                               die "ERROR: libx264 must be installed and version must be >= 0.118."; } &&
                             { check_cpp_condition x264.h "X264_MPEG2" &&
                               enable libx262; }
```

The `use_pkg_config` invokes `pkg-config`. However, `lame` can't be found because it doesn't have a `.pc` file, and configure checks its existence as such:

```sh
enabled libmp3lame        && require "libmp3lame >= 3.98.3" lame/lame.h lame_set_VBR_quality -lmp3lame
```

where this `require` function does a linking check. So any non-keg-only dependencies not using pkg-config are blocked.

> Yeah, it doesn't seem acceptable for a build system to require access to /usr/local/lib as a generic grab bag.

It's not the build system's problem. Without your patch, `/usr/local/lib` goes into `HOMEBREW_LIBRARY_PATHS` (see `determine_library_paths` which I quoted above), which is then processed by our shim `clang` to add `-L/usr/local/lib`. With your path `/usr/local/lib` is stripped from `HOMEBREW_LIBRARY_PATHS`.

So, the obvious fix is to add `opt_lib` of all dependencies to `HOMEBREW_LIBRARY_PATHS`. Other methods in `extend/ENV/super.rb` might need similar treatment too.

, https://git.io/vygE6 in Homebrew/brew
> FFmpeg doesn't use pkg-config.

Wait, it does... I'll take another look at the configure script later.

, https://git.io/vygEi in Homebrew/brew
Not sure why you were able to build `plplot`, but I guess that's because it was able to locate stuff with pkg-config. <s>FFmpeg doesn't use pkg-config.</s>

, https://git.io/vyg0C in Homebrew/brew
I did. Try `ffmpeg` for instance.

, https://git.io/vygYe in Homebrew/brew
linkage_checker: simplify logic for declared deps and include recursive deps
============================================================================

- [x] Have you followed the guidelines in our [Contributing](https://github.com/Homebrew/brew/blob/master/CONTRIBUTING.md) document?
- [x] Have you checked to ensure there aren't other open [Pull Requests](https://github.com/Homebrew/brew/pulls) 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](https://github.com/Homebrew/homebrew/pull/49031).
- [ ] Have you successfully run `brew tests` with your changes locally?

-----

Use `Formula#runtime_dependencies` to collect dependencies.

This inevitably pulls in all recursive deps, which might be controversial (I think this was discussed during the `:linked` dependency days, and no conclusion was reached; the discussion died off after `:linked` was dropped, but dependency resolution has improved since then, so it's worth taking another look), but IMO is a good idea.

Concrete example: in the case of `ffmpeg`, depending on either `theora` or `libvorbis` (both of which has a hard dependency on `libogg`) would introduce a `libogg` linkage, which is currently listed under "possible undeclared dependencies".

, https://git.io/vygYv in Homebrew/brew
> ```
> diff --git a/Library/Homebrew/sandbox.rb b/Library/Homebrew/sandbox.rb
> index 9597daf..b0d6059 100644
> --- a/Library/Homebrew/sandbox.rb
> +++ b/Library/Homebrew/sandbox.rb
> @@ -165,6 +165,9 @@ class Sandbox
>            (regex #"^/dev/ttys?[0-9]*$")
>            )
>        (deny file-write*) ; deny non-whitelist file write operations
> +      (deny file-read*
> +          (literal "/usr/local/lib")
> +      )
>        (allow process-exec
>            (literal "/bin/ps")
>            (with no-sandbox)
> ```

This approach is commendable, but the patch is currently broken because library paths in superenv relies on `#{HOMEBREW_PREFIX}/lib`:

```rb
  def determine_library_paths
    paths = keg_only_deps.map { |d| d.opt_lib.to_s }
    paths << "#{HOMEBREW_PREFIX}/lib"
    paths += homebrew_extra_library_paths
    puts paths
    puts paths.to_path_s
    paths.to_path_s
  end
```

Blocking access to `/usr/local/lib` blocks libraries of all non-keg-only dependencies.

, https://git.io/vyEyH in Homebrew/brew
@MikeMcQuaid Updated according to review comments, with one comment left unaddressed due to reason outlined above.

, https://git.io/vyEyQ in Homebrew/brew
That was the first thing I tried. It looked kinda hideous though, and the second line is easily ignored.

Nonstandard bump subject also sets the precedent for back-to-back opoos for the same issue:

```rb
          opoo "Nonstandard bump subject: #{orig_subject}"
          opoo "Subject should be: #{bump_subject}"
```

```
Warning: Nonstandard bump subject: imagemagick: bump homebrew mirror url for testing
Warning: Subject should be: imagemagick 7.0.5-404
```

, https://git.io/vyEy7 in Homebrew/brew
Can't, shadowing outer local variable. Renamed to `mirror_url` though.