Provide fails_with support for C++11 and OpenMP #22912

Closed
wants to merge 1 commit into
from

4 participants

@mistydemeo

This PR adds support for declaring requirements on language standards: for instance C++11 or OpenMP. It does that using the new DSL method fails_without, which expands to multiple fails_with statements. For instance,

fails_without :cxx11

Translates to fails_with for :gcc, :gcc_4_0, :llvm, :clang build 421 and earlier, and GCC 4.7 and earlier.

Please let me know if you have any suggestions on naming. I had considered placing this in depends_on, but unfortunately the dependency expansion code happens outside the formula context so I don't think that's possible.

To support that, the PR also adjusts the fails_with syntax for non-GCC compilers to be more flexible and more like the Apple compilers. It now comes in the format:

fails_with :gcc => '4.8' do
  release '4.8.0'
end

I don't think the old syntax was in use anywhere yet, and GCC support is still marked as experimental, so this shouldn't break anything.

@MikeMcQuaid MikeMcQuaid and 1 other commented on an outdated diff Sep 28, 2013
Library/Homebrew/formula.rb
@@ -775,11 +776,70 @@ def keg_only reason, explanation=nil
@keg_only_reason = KegOnlyReason.new(reason, explanation.to_s.chomp)
end
+ # For Apple compilers, this should be in the format:
+ # fails_with compiler do
+ # cause "An explanation for why the build doesn't work."
+ # build "The Apple build number for the newest incompatible release."
+ # end
+ #
+ # The block may be omitted, and if present the build may be omitted;
+ # if so, then the compiler will be blacklisted for *all* versions.
+ #
+ # For GNU GCC compilers, this should be in the format:
+ # fails_with compiler => version do
+ # cause
+ # release "The official release number for the latest incompatible
+ # version, for instance 4.8.1"
@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Sep 28, 2013

Not sure I understand why we need version/build/release?. Certainly the last two seem similar or mergeable?

@mistydemeo
mistydemeo added a line comment Sep 28, 2013

build is specific to Apple compilers - e.g. for GCC 4.2.1 build 5666.3, the major release is 4.2, minor release is 4.2.1, and build is 5666. We only really pay attention to build for Apple compilers but it's not quite the same thing.

@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Sep 28, 2013

What is release then (as opposed to version)?

@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Sep 28, 2013

To me more exact: can you explain what build, release and version all are? (Sorry to be annoying).

@mistydemeo
mistydemeo added a line comment Sep 28, 2013

Haha, this shows I should name them better :D

Version: Really "major release", so I should rename this. For GCC 4.8.1, version is "4.8".

Release: Really "minor release". For GCC 4.8.1, release is 4.8.1.

Build: Apple's build number for a given compiler. For instance, for clang, the latest build is 500. It's not really a "version" number because they track it separately but they often introduce substantive changes even if they only bump the build and not any other numbers.

@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Sep 28, 2013

Ok. I guess to me the version and release feel like they should be somewhat combined and/or build and release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@MikeMcQuaid
Homebrew member

Great work. Other than version/build/release this looks good to go to me.

@MikeMcQuaid
Homebrew member

Tests failing too.

@jacknagel jacknagel commented on an outdated diff Sep 28, 2013
Library/Homebrew/formula.rb
+ end
+ end
+ end
+ when :openmp
+ [:clang, :gcc, :gcc_4_0, :llvm].each do |cc|
+ fails_with cc do
+ cause 'This formula requires an OpenMP-compatible compiler.'
+ end
+ end
+ else
+ raise ArgumentError.new <<-EOS.undent
+ Unrecognized standard: #{standard}
+ Recognized values are: :cxx11, :openmp
+ EOS
+ end
+ end
@jacknagel
jacknagel added a line comment Sep 28, 2013

This is a lot of logic to embed in a DSL method, which suggest to me that there should be some intermediate object that collects these things.

@jacknagel
jacknagel added a line comment Dec 6, 2013

To provide a more concrete suggestion, how about a class method on CompilerFailure that does this work?

Then this method could be something like

def fails_without standard
  @cc_failures ||= Set.new
  @cc_failures.merge CompilerFailure.failures_for_standard(standard)
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@manphiz manphiz and 1 other commented on an outdated diff Sep 30, 2013
Library/Homebrew/formula.rb
+ cause 'This formula requires a C++11-compatible compiler.'
+ end
+
+ fails_with :clang do
+ build 421
+ cause 'This formula requires C++11 support available in clang build 421 or newer.'
+ end
+
+ (3..7).each do |v|
+ fails_with "gcc-4.#{v}" do
+ cause 'This formula requires C++11 support, which is available in GCC 4.8 or newer.'
+ end
+ end
+ end
+ when :openmp
+ [:clang, :gcc, :gcc_4_0, :llvm].each do |cc|
@manphiz
manphiz added a line comment Sep 30, 2013

I was under the impression that gcc and llvm are OpenMP-compatible, no?

@mistydemeo
mistydemeo added a line comment Oct 6, 2013

You're right - I thought only newer GCCs were, but it looks like gcc-4.2 and llvm-gcc are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacknagel

Working through compiler issues the last few days, I've been thinking about this a lot, and I'm not sure that I like the way fails_without <some feature> reads. I think requires :openmp or requires :cxx11 would be more clear (not sure how others feel about "requires", but you get the idea). What do you think?

@mistydemeo

I agree that I really don't like fails_without. requires overloads the existing Requirement feature, but how about something like standard :cxx11, requires_standard :cxx11, needs_standard :cxx11, etc.?

@mistydemeo

Adjusted the fails_with code for non-Apple compilers. Terminology slightly cleared up: fails_with now cares about version and major_version for these. major_version is, for example, 4.8 for GCC 4.8.2.

Poking at unifying build and version, if I can come up with something I'm happy with.

@mistydemeo

Alternate implementation: mistydemeo@f57a5de

This continues to expose build publicly for fails_with, but internally treats build and version as synonyms. Since build number is really the version number we care about for Apple compilers, I don't feel this is too misleading. Internally code's been migrated to use version instead of build for Apple compilers too. major_version currently does nothing for Apple compilers because we treat their version numbers specially.

@mistydemeo

fails_with updated in mistydemeo@b9d8089

Tempted to push that commit soon, and play around with the rest a little more.

@mistydemeo

Going to merge mistydemeo@b9d8089 tonight.

@jacknagel

I've come back around on fails_without as a name, for this reason: we're unlikely to wish we could use it for something else, so if we later deprecate it in favor of something else, it won't be a huge deal. Whereas something like standard is much more broad, and users might expect it to do things other than just add fails_with annotations.

Long-term, we can package all of the smaller pieces up into something like variants: https://gist.github.com/jacknagel/6731244

@bredelings bredelings commented on an outdated diff Jan 14, 2014
Library/Homebrew/formula.rb
+ # The opposite of fails_with: marks a requirement on a given
+ # standard, and automatically registers failures for the
+ # compilers that don't support it.
+ # Currently supported:
+ # * :cxx11
+ # * :openmp
+ def fails_without standard
+ case standard
+ when :cxx11
+ [:gcc, :gcc_4_0, :llvm].each do |cc|
+ fails_with cc do
+ cause 'This formula requires a C++11-compatible compiler.'
+ end
+
+ fails_with :clang do
+ build 421
@bredelings
bredelings added a line comment Jan 14, 2014

I think some C++11 features are only available in build 500 or newer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mistydemeo mistydemeo added a commit to mistydemeo/homebrew that referenced this pull request Apr 5, 2014
@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
c60a509
@mistydemeo

So I finally got around to doing this a mere FOUR MONTHS LATER. It's now named needs, and is handled by a class method on CompilerFailure.

@mistydemeo mistydemeo added a commit to mistydemeo/homebrew that referenced this pull request Apr 5, 2014
@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
6096fe4
@adamv adamv and 1 other commented on an outdated diff Apr 5, 2014
Library/Homebrew/formula.rb
@@ -797,6 +797,11 @@ def fails_with compiler, &block
@cc_failures << CompilerFailure.new(compiler, &block)
end
+ def needs standard
@adamv
adamv added a line comment Apr 5, 2014

Should this take a list (*standards)?

@mistydemeo
mistydemeo added a line comment Apr 5, 2014

Good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mistydemeo mistydemeo added a commit to mistydemeo/homebrew that referenced this pull request Apr 5, 2014
@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
c92d3c6
@MikeMcQuaid
Homebrew member

👍

@mistydemeo

@BrewTestBot test this please

@mistydemeo

@BrewTestBot test this please

@mistydemeo

@Homebrew/owners I'm going to merge this soon.

@mistydemeo mistydemeo added a commit to mistydemeo/homebrew that referenced this pull request Apr 12, 2014
@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
99cd09a
@jacknagel jacknagel commented on an outdated diff Apr 12, 2014
Library/Homebrew/formula.rb
@@ -761,6 +761,13 @@ def fails_with compiler, &block
@cc_failures << CompilerFailure.new(compiler, &block)
end
+ def needs *standards
+ standards.each do |standard|
+ @cc_failures ||= Set.new
@jacknagel
jacknagel added a line comment Apr 12, 2014

I'd pull this up out of the loop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jacknagel

LGTM

@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
fd14370
@mistydemeo mistydemeo added a commit that closed this pull request Apr 12, 2014
@mistydemeo mistydemeo Formula: provide compiler failure collections
`needs` allows formulae to specify dependencies on cross-compiler
dependencies, allowing multiple failures to be specified in a single
statement. For instance, `needs :cxx11` adds seven compiler failures.

Closes #22912.
15ca054
@mistydemeo mistydemeo deleted the mistydemeo:compilers branch Apr 12, 2014
@adamv

Seeing new audits:

$ brew audit
Error: Failed to import: aspcud
can't convert nil into String
Error: Failed to import: capnp
can't convert nil into String
Error: Failed to import: libcppa
can't convert nil into String
Error: Failed to import: mal4s
can't convert nil into String
mapserver:
 * Dependency gd does not define option "with-freetype"

Error: Failed to import: mkvdts2ac3
can't convert nil into String
Error: Failed to import: mpd
can't convert nil into String
Error: Failed to import: sdcv
can't convert nil into String
Error: 1 problems in 1 formulae
@mistydemeo

Just fixed those now.

@MikeMcQuaid MikeMcQuaid commented on the diff Apr 14, 2014
Library/Homebrew/compilers.rb
+ [{:gcc => '4.4'}, proc { cause MESSAGES[:cxx11] }],
+ [{:gcc => '4.5'}, proc { cause MESSAGES[:cxx11] }],
+ [{:gcc => '4.6'}, proc { cause MESSAGES[:cxx11] }]
+ ],
+ :openmp => [
+ [:clang, proc { cause 'clang does not support OpenMP' }]
+ ]
+ }
+
+ def self.for_standard standard
+ failures = COLLECTIONS.fetch(standard) do
+ raise ArgumentError, "\"#{standard}\" is not a recognized standard"
+ end
+
+ failures.map do |compiler, block|
+ CompilerFailure.new(compiler, &block)
@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Apr 14, 2014

@mistydemeo Noticed a problem with this: it ends up raising a CompilerSelectionError which recommends installing apple-gcc42 which won't fix the issue. Additionally, it ends up raising an error on 10.7 and below which cannot be installed by any formulae in core. I guess we/I need to speed up the replacement of apple-gcc42 in core with a proper gcc formula but we might want to handle this messaging before then.

@mistydemeo
mistydemeo added a line comment Apr 14, 2014

The messaging needs work, yeah... unfortunately the CompilerSelectionError isn't currently aware of why there are no suitable compilers so it's hard for us to make a better recommendation. Maybe it's better just to recommend something up-to-date from homebrew/versions, e.g. gcc48.

@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Apr 14, 2014

I'd say if we can avoid making any recommendation in this particular case somehow it'd be better than a bad one. I'd rather we didn't recommend stuff from outside of core yet. I'll try and get this GCC branch up.

@mistydemeo
mistydemeo added a line comment Apr 14, 2014

We have no idea what the case is when CompilerSelectionError occurs (because the only reason metadata is human-readable). We could remove all recommendations.

@MikeMcQuaid
Homebrew member
MikeMcQuaid added a line comment Apr 14, 2014

Sure but I guess my point was we could refactor the could so we do know what case it is ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.