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

Fix #4293: Use a stricter superenv for CMake builds #4308

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zbeekman
Copy link
Member

zbeekman commented Jun 7, 2018

  • HOMEBREW_PREFIX.to_s was getting added to the CMake environment variable:
    CMAKE_PREFIX_PATH which causes CMake to look for stuff in:
    HOMEBREW_PREFIX.to_s/{bin,lib,share,include,etc,...}

  • This can cause Homebrew to pick up alternate fortran compilres if you provie
    links to them in e.g., /usr/local/bin assuming you installed homebrew in
    the standard location.

  • This may cause problems with libraries, programs, headers etc. getting picked
    up that aren't enumerated as formula dependencies.

  • Fixes #4293

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

No. My ruby isn't super strong, and I looked at the existing tests in Library/Homebrew/test both ENV_spec.rb and build_environment_spec.rb and it wasn't clear how to write a meaningful test for this small change. The big test will be to see if all formulae with depends_on "cmake" => :build can build cleanly and then to determine if they fail because the formula is not specifying dependencies correctly, or if there is some deeper reason as to why the homebrew prefix was included in the CMAKE_PREFIX_PATH to begin with.

  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Use a stricter superenv for CMake builds
 - `HOMEBREW_PREFIX.to_s` was getting added to the CMake environment variable:
   `CMAKE_PREFIX_PATH` which causes CMake to look for stuff in:
   `HOMEBREW_PREFIX.to_s/{bin,lib,share,include,etc,...}`
 - This can cause Homebrew to pick up alternate fortran compilres if you provie
   links to them in e.g., `/usr/local/bin` assuming you installed homebrew in
   the standard location.
 - This may cause problems with libraries, programs, headers etc. getting picked
   up that aren't enumerated as formula dependencies.
 - Fixes #4293
@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 7, 2018

brew tests has a bunch of failures locally, but I don't understand why. I suspect my ruby gems environment may be messed up (or there are issues with my case-sensitive FS). Manually trying some of the stuff the test failures report seems to indicate that these are false positives.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 7, 2018

The build fialures on CI are due to the output being different than the expected output when dumping the environment. If someone could point me in the right direction for updating these fixtures/acceptance tests that would be great.

`CMAKE_PREFIX_PATH` no longer expected in ENV spec
 - Fix tests so that they no longer expect to see `CMAKE_PREFIX_PATH`

@zbeekman zbeekman force-pushed the zbeekman:stricter-cmake-superenv-#4293 branch from 55fe26e to a3500de Jun 7, 2018

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 7, 2018

I think I updated the tests appropriately. I had a dumb style error after updating the tests. If these pass, then LGTM.

@@ -1,43 +1,38 @@
describe "brew --env", :integration_test do
it "prints the Homebrew build environment variables" do
expect { brew "--env" }
.to output(/CMAKE_PREFIX_PATH="#{Regexp.escape(HOMEBREW_PREFIX)}[:"]/).to_stdout

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 7, 2018

Member

Might be good to verify something else from the output here e.g. CC="clang"?

This comment has been minimized.

@zbeekman

zbeekman Jun 7, 2018

Member

I don't see CC being set either from brew --env or brew install --env=super --interactive hello then doing brew --env. Here are the options that are available to me, AFAICT:

HOMEBREW_CC: clang
HOMEBREW_CXX: clang++
MAKEFLAGS: -j4
CMAKE_INCLUDE_PATH: /usr/include/libxml2:/System/Library/Frameworks/OpenGL.framework/Versions/Current/Headers
CMAKE_LIBRARY_PATH: /System/Library/Frameworks/OpenGL.framework/Versions/Current/Libraries
PKG_CONFIG_LIBDIR: /usr/lib/pkgconfig:/usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig/10.13
ACLOCAL_PATH: /usr/local/share/aclocal
PATH: /usr/local/Homebrew/Library/Homebrew/shims/mac/super:/usr/bin:/bin:/usr/sbin:/sbin

So the best ones to verify might be:

  • HOMEBREW_CC matches clang
  • HOMEBREW_CXX matches clang++

and then we could also check that PATH contains each of:

  • /usr/bin
  • /bin
  • /usr/sbin
  • /sbin

What do you think @MikeMcQuaid? I think I might be capable of implementing that with my limited ruby. Also please let me know if I should squash everything, or use multiple commits.

Not so sure why CMAKE_LIBRARY_PATH and CMAKE_INCLUDE_PATH are getting set, but it's not harming me, so I'll leave that alone.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2018

Member

HOMEBREW_CC: clang, sorry. It's sufficient to check that instead of CMAKE_PREFIX_PATH.

and then we could also check that PATH contains each of:

Don't think we need to do that.

This comment has been minimized.

@zbeekman

zbeekman Jun 8, 2018

Member

OK, so no expansion of the env tests beyond HOMEBREW_CC=clang? Could be good to test ACLOCAL_PATH has content since the env tests are fairly sparse, but I'm happy leaving it alone too.

@@ -190,7 +190,6 @@ def determine_dependencies
def determine_cmake_prefix_path
PATH.new(
keg_only_deps.map(&:opt_prefix),
HOMEBREW_PREFIX.to_s,

This comment has been minimized.

@DomT4

DomT4 Jun 8, 2018

Contributor

Potentially stupid question, but if we're removing this line shouldn't we be tweaking the line above to pass all recursive deps via the opt_prefix instead of purely the keg_only ones?

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 8, 2018

Something like recursive_dependencies.map(&:opt_prefix) might work, or maybe even deps.map(&:opt_prefix) if we don't need to explicitly include everything through the tree. Suspect this is going to need some testing to check out.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 8, 2018

As far as I'm aware, not with this change applied first unless it was merged, but I could be wrong on that front. It's been a while since I discussed CI stuff with anyone here so you may need Mike/Joe/Misty/etc to weigh in on that.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 8, 2018

Yeah I don't think we want to remove that entirely but moving its priority should be ok.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 8, 2018

Yeah I don't think we want to remove that entirely

Remove what entirely, sorry; HOMEBREW_PREFIX.to_s? I do think the more correct behaviour is to, as @DomT4, pass through the dependencies instead.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 8, 2018

Remove what entirely, sorry; HOMEBREW_PREFIX.to_s? I do think the more correct behaviour is to, as @DomT4, pass through the dependencies instead.

Correct. This is the same sort of change that was very breaking when we were experimenting with sandboxing out /usr/local. I'm assuming that changing the priority fixes the local source build issue that @zbeekman originally identified, and I don't see any CI build failures just removing it entirely is intended to fix unless I've missed something.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 8, 2018

I don't see any CI build failures just removing it entirely is intended to fix unless I've missed something.

Fair point 👍. Will wait further for @zbeekman to confirm either way.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

@ilovezfs

Yeah I don't think we want to remove that entirely but moving its priority should be ok.
...
I'm assuming that changing the priority fixes the local source build issue that @zbeekman originally identified

I'm not sure I quite understand this comment. Move what's priority? How? I can confirm that not setting CMAKE_PREFIX_PATH in the build environment sorted out the issue I was experiencing.

Are you suggesting that we ensure HOMEBREW_PREFIX is the right-most entry in CMAKE_PREFIX_PATH and then also explicitly enumerate the dependencies ahead of it?

@MikeMcQuaid

Fair point 👍. Will wait further for @zbeekman to confirm either way.

Confirm what? Breakage or no breakage of formula builds from local testing when removing HOMEBREW_PREFIX and using explicit enumeration of dependencies instead?

I'm happy to do some local testing and try to implement this, but a little bit more clarification addressing the points above would be much appreciated!

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 8, 2018

Are you suggesting that we ensure HOMEBREW_PREFIXis the right-most entry in CMAKE_PREFIX_PATH and then also explicitly enumerate the dependencies ahead of it?

@zbeekman I believe so but @ilovezfs will confirm.

Confirm what?

Confirm that the "ensure HOMEBREW_PREFIXis the right-most entry" still fixes your problems.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

Having CMAKE_PREFIX_PATH=...:/usr/local still seems like an excessively large hammer to me for a sanitized build environment. Autotools builds don't appear to have the same level of promiscuousness enabled in their build environments. However, I will defer to the ever-wise judgement of the Homebrew maintainers; I know how it is when a contributor makes a change that leads to unforeseen problems that are a headache to cleanup later.

I'm going to wait for clarification from @ilovezfs as discussed above before proceeding with further the implementation.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 8, 2018

@zbeekman I believe so but @ilovezfs will confirm.

Yes.

Having CMAKE_PREFIX_PATH=...:/usr/local still seems like an excessively large hammer to me for a sanitized build environment. Autotools builds don't appear to have the same level of promiscuousness enabled in their build environments.

I'm concerned about two main scenarios

  1. unexpected build failures (449 formulae use cmake)
  2. seemingly successful builds that silently end up no longer building against all of their dependencies

If we're confident that enumerating all of the recursive dependencies will avoid those, then I'd be willing to experiment with leaving /usr/local off the list altogether.

There's also the question what to do in the case of runtime dependencies of build time dependencies See #3883 for background.

We leave their bin directories out of the PATH, but because we've been setting CMAKE_PREFIX_PATH they'd likely have been found without needing to be enumerated. Would you be including them in the recursive list or not? If not, we should expect to need to add some more direct build deps to formulae.

Let the Whac-A-Mole games begin!

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 8, 2018

I'm concerned about two main scenarios

unexpected build failures (449 formulae use cmake)
seemingly successful builds that silently end up no longer building against all of their dependencies
If we're confident that enumerating all of the recursive dependencies will avoid those, then I'd be willing to experiment with leaving /usr/local off the list altogether.

There's also the question what to do in the case of runtime dependencies of build time dependencies See #3883 for background.

We leave their bin directories out of the PATH, but because we've been setting CMAKE_PREFIX_PATH they'd likely have been found without needing to be enumerated. Would you be including them in the recursive list or not? If not, we should expect to need to add some more direct build deps to formulae.

Let the Whac-A-Mole games begin!

@ilovezfs thanks for the response! Given my limited time, while I think removing usr/local/ from CMAKE_PREFIX_PATH is the right approach, I'm going to start off by seeing if pre-pending CMAKE_PREFIX_PATH with Formula deps (possibly with an upstream CMake improvement/fix required to get it working correctly as identified in https://gitlab.kitware.com/cmake/cmake/issues/18072) will improve CMake's behavior enough for most people's needs.

My vote would be to open a tracking issue or something similar for the more extensive work and validation that would be required to completely remove HOMEBREW_PREFIX from CMAKE_PREFIX_PATH.

@scpeters

This comment has been minimized.

Copy link
Contributor

scpeters commented Jun 8, 2018

I hadn't said anything before, but I was feeling a bit nervous about removing /usr/local from CMAKE_PREFIX_PATH entirely, as we maintain several formulae that use cmake in a tap, and you never know what will happen with a change like this. If appending to CMAKE_PREFIX_PATH instead of pre-pending fixes the original fortran selection issue, that sounds safest to me at this point.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 9, 2018

Autotools builds don't appear to have the same level of promiscuousness enabled in their build environments.

The way we force the autotools to behave is more logically sane and less maintainer/contributor-intensive, but sadly attempting to force CMake to that same point is hella risky now because this is the established setup for CMake and lots of people have made lots of (both accidental and deliberate) assumptions that that is unlikely to change.

If we're confident that enumerating all of the recursive dependencies will avoid those, then I'd be willing to experiment with leaving /usr/local off the list altogether.

I do think this is worth a go, though, potentially via this sort of tweak. If we go for the broadest possible CMAKE_PREFIX_PATH that isn't simply the entirety of HOMEBREW_PREFIX that seems like a viable path forwards. It is going to take a whole bunch of testing though.

because we've been setting CMAKE_PREFIX_PATH they'd likely have been found without needing to be enumerated. Would you be including them in the recursive list or not?

I think it's likely we'd have to, at least initially.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 9, 2018

The way we force the autotools to behave is more logically sane and less maintainer/contributor-intensive, but sadly attempting to force CMake to that same point is hella risky now because this is the established setup for CMake and lots of people have made lots of (both accidental and deliberate) assumptions that that is unlikely to change.

It doesn't need to be hella risky, though. We can enable this only if an environment variable is set, set it in CI and test the relevant formulae that way. After that we can set it just for HOMEBREW_DEVELOPER folks then just master branch and then everyone.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 9, 2018

seemingly successful builds that silently end up no longer building against all of their dependencies

except that CI won't notice this.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 10, 2018

seemingly successful builds that silently end up no longer building against all of their dependencies

except that CI won't notice this.

It won't but I don't see any basis for that happening due to a combination of how Homebrew and CMake both work:

  • we link everything in bin/lib to HOMEBREW_PREFIX (without except) and we are pointing to the unlinked versions if we add them to CMAKE_PREFIX_PATH
  • CMAKE_PREFIX_PATH is used by every find_* invocation in CMake so if you have the same files being linked from a keg to HOMEBREW_PREFIX and you provide the keg instead of the HOMEBREW_PREFIX there's no reason to believe CMake would be unable to find the path there

As a more general point I personally don't really like the "we shouldn't change this because it might break something" argument as it can (and has been) applied to almost all major (and a bunch of minor) Homebrew changes that have ended up being for the better. As-is this code is making opportunistic linking practically impossible to avoid in CMake's case as we're actively providing it with the means to avoid superenv.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 10, 2018

and you provide the keg instead of the HOMEBREW_PREFIX

Except that isn't what's being proposed if the runtime dependencies of build time dependencies are not added.

Homebrew changes that have ended up being for the better

This is in the eye of the beholder given the amount of time we spend fighting with superenv as it is already.

As a more general point I personally don't really like the "we shouldn't change this because it might break something" argument

The point is that the effectiveness of the phase-it-in-via-an-envvar-set-in-CI plan reducing risk depends on CI actually detecting problems, which it does in the case of the sandbox violations, but does not in the case of changed linkage.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 10, 2018

Also, note superenv already relying heavily on HOMEBREW_PREFIX for non-keg-only stuff:

keg_only_deps.map { |d| d.opt_share/"aclocal" },
HOMEBREW_PREFIX/"share/aclocal",

HOMEBREW_PREFIX/"include",

PATH.new(keg_only_deps.map(&:opt_include)).existing

paths = [
keg_only_deps.map(&:opt_lib),
HOMEBREW_PREFIX/"lib",
]

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 10, 2018

I personally don't really like the "we shouldn't change this because it might break something" argument as it can

FWIW this wasn't so much my point in my earlier comment.

I think this would be a change for the better ultimately, but I also think there's a tricky middle ground to find between ensuring this is tested broadly enough to ever be able to roll it out for everyone on anything like a reasonable timescale, and yet also ensuring it isn't tested so broadly core is suddenly pounded by build failure issues.

Also, note superenv already relying heavily on HOMEBREW_PREFIX for non-keg-only stuff:

Again FWIW, I think this sort of thing should be phased out as well, personally. Long-term it'd resolve an absolute ton of hassle.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 10, 2018

Again FWIW, I think this sort of thing should be phased out as well, personally. Long-term it'd resolve an absolute ton of hassle.

Sure. My point was that there's nothing remotely pure about the non-CMake bits in superenv right now so CMake is not uniquely defeating anything as superenv already defeats itself.

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jun 10, 2018

FYI, this is my current plan:

  • Promote build & runtime deps recursively ahead of HOMEBREW_PREFIX on CMAKE_PREFIX_PATH
  • Enhance tests
  • Check if my toolchain/compiler issue is resolved

If this all goes to plan, I suggest we merge this PR and open a new tracking issue or PR to make homebrew superenv less promiscuous & more pristine for CMake where we can experiment with removing HOMEBREW_PREFIX from CMAKE_PREFIX_PATH entirely and various other approaches discussed here. If anyone objects please let me know, I hope to look at this a bit later this afternoon.

Thanks

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Jun 10, 2018

Yes that makes sense. Let's get the minimal diff to resolve #4293 without potentially breaking anything else merged ASAP :)

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Jun 10, 2018

Aye, that sounds 👍. I'll have some more time available imminently so the plan/hope is to pick up a few more things around brew that I've been meaning to go after for a while, hopefully being more useful than being primarily a commenter lately.

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 1, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jul 1, 2018

@zbeekman

This comment has been minimized.

Copy link
Member

zbeekman commented Jul 1, 2018

@stale stale bot removed the stale label Jul 1, 2018

@stale

This comment has been minimized.

Copy link

stale bot commented Jul 22, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jul 22, 2018

@stale stale bot closed this Jul 29, 2018

@lock lock bot added the outdated label Aug 28, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 28, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.