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

Closed
mistydemeo opened this Issue Mar 27, 2013 · 8 comments

Projects

None yet

4 participants

@mistydemeo

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.

@mistydemeo

See mistydemeo/tigerbrew#15 for an example.

@mistydemeo mistydemeo referenced this issue in mistydemeo/tigerbrew Mar 27, 2013
Closed

mpfr failed to build on 10.4.11 #15

@MikeMcQuaid
Homebrew member

Seems sensible to me.

@jacknagel

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

@jacknagel

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

@mistydemeo

(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
@jacknagel

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.

@mistydemeo

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

@mistydemeo mistydemeo added a commit to mistydemeo/homebrew that referenced this issue Aug 23, 2013
@mistydemeo 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
@apjanke apjanke modified the milestone: Clear out Legacy May 3, 2016
@MikeMcQuaid
Homebrew member

I think we can close this out at this point; it seems it's not being either addressed or causing major problems.

@MikeMcQuaid MikeMcQuaid locked and limited conversation to collaborators Jul 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.