Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

MPIRequirement does not work with superenv #16409

Closed
wants to merge 1 commit into from

3 participants

@samueljohn

On superenv, it either fails to find the mpicc (and others) or the mpicc itself fails to find the cc.
To solve this, we need to setup a little temporary build environment.
I made sure that the PATH and other things are resetted, after the check of the requirement.

This is not a final and polished implementation but rather something at hand to discuss.

@samueljohn

ping @Sharpie b/c you wrote the requirement initially. Good work. What do you think?
Can this be made simpler than in my quick hack?

@jacknagel
Owner

Pushed #16733, requirements can now specify env :userpaths. Does that cover this?

@samueljohn

I'll test (soonish: little spare time until next year though)

@scicalculator scicalculator referenced this pull request in Homebrew/homebrew-science
Closed

LAMMPS: New formula request #45

@samueljohn

@jacknagel, even with #16733 it seems as it does not work with superenv on Xcode-only systems.

I get some output like this:

/homebrew/bin/mpicxx --version
Homebrew could not locate working copies of the following MPI compiler
wrappers:
    mpicxx

If you have a MPI installation, please ensure the bin folder is on your
PATH and that all the wrappers are functional. Otherwise, a MPI
installation can be obtained from homebrew by *picking one* of the

But

which mpicxx
/homebrew/bin/mpicxx
@samueljohn

@jacknagel updated, rebased and tested.

In an ideal world, satisfied? would be called in the environment that is identical as during the build. But that is not so easy to implement. For MPIRequirement, we can hack this by calling ENV.userpaths! (and restoring to the original ENV['PATH'] after the satisfied-test.

@jacknagel
Owner

I don't understand why this is necessary, and I think that means there is a subtler bug here. This is my understanding of what should be happening:

superenv ignores the inherited value of PATH and constructs a new value. The paths injected by calling ENV.userpaths! are paths that were in PATH when the main Homebrew process was kicked off. But superenv does not reset PATH in the main Homebrew process; the superenv code is only loaded by the forked build process.

Requirement#satisified? is only ever called by the main Homebrew process (during dependency expansion, in FormulaInstaller, before forking the build process). So calling ENV.userpaths! here should be a no-op; those paths should still be in PATH.

@jacknagel
Owner

I will try to figure it out when I get home.

@samueljohn

Perhaps its more subtle. It is not the mpicc that is not found - it is the cc. Let me explain:

With Xcode-only, I don't have the compiler in my PATH. Both superenv and standard-env work around this by adding the xctoolchain dir to the PATH.

While I have /homebrew/bin/mpicc in my path, mpicc --version fails because it cannot find cc.

@jacknagel
Owner

I see. But wouldn't we need to invoke superenv or setup_build_environment here to get the toolchain path into PATH?

@samueljohn

My first version just called ENV.setup_build_environment but that was not enough. It does not set the userpaths, so I had to call ENV.userpaths! anyway.

@samueljohn

Ideally, statisfied? should be executed in the same env as the formula will be build (for all Requirements). I found that veeeery hard to code.

@jacknagel you are right, the PATH to that time does not include the Library/ENV/4.3 dir.
mpicxx --version outputs to stderr:

--------------------------------------------------------------------------
The Open MPI wrapper compiler was unable to find the specified compiler
g++ in your PATH.

Note that this compiler was either specified at configure time or in
one of several possible environment variables.
--------------------------------------------------------------------------

However quiet_system still thinks it has found a working mpixcc.

So what now?
Just let the satisfied? test pass by only adding ENV.userpaths! (as is in my PR right now), which will add HOMEBREW_PREFIX/bin to the PATH (if the user has set its PATH to homebrew correctly).

Or also call ENV.setup_build_environment as you suggest. But then again, this will set many variables and potentially influence other Requirements and so forth.

@jacknagel
Owner

Just let the satisfied? test pass by only adding ENV.userpaths! (as is in my PR right now), which will add HOMEBREW_PREFIX/bin to the PATH (if the user has set its PATH to homebrew correctly).

But this is my original gripe: when satisfied? is called, we haven't even removed HOMEBREW_PREFIX/bin from the path. We haven't removed anything from the path. PATH hasn't been changed at all.

@samueljohn

Ok. fuck. I was bitten by Ruby's "return the last thing". LOL.

@samueljohn

Explicit is better than implicit. duh.

@samueljohn

TIL: @jacknagel is always right.

Please see updated diff.

@jacknagel
Owner

Aha! I should have noticed that.

I think this will be OK for now. If we need this for more requirements we can probably cook up something that takes a block and evals it in the context of the build environment, and then restores the original env when returning.

Will pull later on.

@Sharpie Sharpie referenced this pull request
Closed

Fix and Update scotch #16977

@Sharpie
Collaborator

This looks like a good patch for the moment. The other workaround I had in mind for this was to re-work the test so it only uses Homebrew-installed versions of Open-MPI and MPICH2 instead of "whatever is laying around in the PATH" and then adds the LinkedKeg bin folders instead of everything that gets drug in by :userpaths.

In an ideal world, satisfied? would be called in the environment that is identical as during the build.

Very true. The idea of testing components outside of the environment in which we expect them to execute their tasks is fundamentally broken.

@jacknagel jacknagel closed this pull request from a commit
@samueljohn samueljohn Make MPIRequirement satisfied on Xcode-only Macs
Closes #16409.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
e13842a
@jacknagel jacknagel closed this in e13842a
@jacknagel
Owner

Pulled.

@samueljohn

Thank you. On a side note: We get email notifications when a PR is automatically closed from a commit.

@Sharpie Sharpie referenced this pull request from a commit in Sharpie/homebrew
@samueljohn samueljohn Make MPIRequirement satisfied on Xcode-only Macs
Closes #16409.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
0283264
@dholm dholm referenced this pull request from a commit in dholm/homebrew
@samueljohn samueljohn Make MPIRequirement satisfied on Xcode-only Macs
Closes #16409.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>
fe7217a
@guyzmo guyzmo referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@cooljeanius cooljeanius referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@rajeeja rajeeja referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 10, 2013
  1. @samueljohn
This page is out of date. Refresh to see the latest.
Showing with 8 additions and 0 deletions.
  1. +8 −0 Library/Homebrew/requirements.rb
View
8 Library/Homebrew/requirements.rb
@@ -124,6 +124,11 @@ def mpi_wrapper_works? compiler
end
def satisfied?
+ # we have to assure the ENV is (almost) as during the build
+ orig_PATH = ENV['PATH']
+ require 'superenv'
+ ENV.setup_build_environment
+ ENV.userpaths!
@lang_list.each do |lang|
case lang
when :cc, :cxx, :f90, :f77
@@ -134,6 +139,9 @@ def satisfied?
end
end
+ # Restore the original paths
+ ENV['PATH'] = orig_PATH
+
@unknown_langs.empty? and @non_functional.empty?
end
Something went wrong with that request. Please try again.