Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

MacOS.(gcc|clang|llvm)_version can return nil #18781

Open
mistydemeo opened this Issue · 7 comments

3 participants

Misty De Meo Mike McQuaid Jack Nagel
Misty De Meo
Owner

If the requested compiler isn't installed, the various MacOS compiler version methods will return nil instead of an int.

While maybe not a bad thing, we test against the outputs of those with <=> fairly frequently and that's going to fail if the result is nil. Leads to some breakage in Tigerbrew, and I imagine it's going to cause problems for us when llvm-gcc is dropped.

We should probably be returning a CompilerVersion class of some kind instead of a bare int, a la MacOS::Version.

Misty De Meo
Owner

See mistydemeo/tigerbrew#15 for an example.

Misty De Meo mistydemeo referenced this issue in mistydemeo/tigerbrew
Closed

mpfr failed to build on 10.4.11 #15

Mike McQuaid
Owner

Seems sensible to me.

Jack Nagel
Owner

Really, it's a shame this stuff leaked into the formula API rather than being wrapped in a higher-level construct.

Jack Nagel
Owner

Well the standard alternative to "value or nil" semantics would be something like Version::NULL that defines <=> to return -1.

But really, any use of these methods should be accompanied by a ENV.compiler == :foo check, as it doesn't make much sense to branch on the compiler version without knowing what compiler is in use (though even that doesn't work in the DSL, which is evaluated before the compiler selection logic kicks in).

Misty De Meo
Owner

(though even that doesn't work in the DSL, which is evaluated before the compiler selection logic kicks in).

Yeah, I thought that was fishy. With the specific mpfr issue, I suppose the real answer is "don't do that". Given that it apparently works with clang build 425 in superenv I'm happy just to mark mpfr fails_with :clang {build 421}.

I can easily enough add a Version::NULL and change the methods to return that instead of nil if no value is found, were you preferring that to a CompilerVersion class?

class Version::NULL
  class << self
    include Comparable
    def <=>(other); -1; end
    def nil?; true; end
  end
end
Jack Nagel
Owner

I thought perhaps a generic null version object would be more reusable. Though a CompilerVersion class would avoid a conditional in these methods, so maybe it's better.

Side note: every time I look at the compiler stuff I feel like it is just begging for extraction into something more OO. I think maybe compilers should eventually be a first-class object.

Misty De Meo mistydemeo referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Misty De Meo
Owner

Recently reminded about this again because of a fails_with failure in Tigerbrew due to comparisons: https://gist.github.com/shirleyallan/6306278

Introducing compiler objects would pry be cleaner, but a quicker fix is: https://github.com/mistydemeo/homebrew/compare/version_null

Misty De Meo mistydemeo referenced this issue from a commit in mistydemeo/homebrew
Misty De Meo mistydemeo Compilers: return comparable NULL when missing
Otherwise code like fails_with will try to compare versions with nil,
which has bad results.

Fixes #18781.
5f48f42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.