Skip to content

fix clang build version on Ubuntu Artful#3943

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
bukzor:clang-build-version
Mar 22, 2018
Merged

fix clang build version on Ubuntu Artful#3943
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
bukzor:clang-build-version

Conversation

@bukzor
Copy link
Contributor

@bukzor bukzor commented Mar 18, 2018

Ported from Linuxbrew/brew#621

Before:

$ brew config | grep Clang:
Clang: 4.0 build 

After:

$ brew config | grep Clang:     
Clang: 4.0 build 401

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indentation change is incorrect; it's part of the if statement above. Also, what is present in the .* before RELEASE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string is clang version 5.0.1 (tags/RELEASE_501/final).
According to the pastebins I was able to find, the "501" is what's found under osx.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks. What about making it tags/RELEASE_ instead of .*?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was less certain that would continue to match, and I don't think the current pattern will over-match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bukzor I'd rather we go for something more conservative for now and extended when needed. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can dooo!

@bukzor bukzor force-pushed the clang-build-version branch 2 times, most recently from 0db9fdd to f08eddb Compare March 20, 2018 21:33
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent is to make it more noticeable when the above regex doesn't match. I think it does a good job of that.
The output in that case looks like so:

Clang: 5.0 build ???

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest Clang: 5.0 build N/A.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Not applicable" is the correct answer when a tool is not installed, which is its usage in the rest of this file, but the build number is indeed applicable when clang is installed, as it is here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the "???" string to "(parse error)", which should be more clear.

Copy link
Contributor Author

@bukzor bukzor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offenses:
Library/Homebrew/development_tools.rb:77:48: C: Use %r around regular expression.
           build_version = `#{path} --version`[/clang(-| version [^ ]+ \(tags\/RELEASE_)(\d{2,})/, 2]
                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Library/Homebrew/system_config.rb:213:64: C: Ternary operators must not be nested. Prefer if or else constructs instead.
      f.puts "Clang: #{clang.null? ? "N/A" : "#{clang} build #{clang_build.null? ? "???" : clang_build}"}"
                                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@MikeMcQuaid
Copy link
Member

@bukzor You can reproduce those errors locally with brew style.

@bukzor bukzor force-pushed the clang-build-version branch from f08eddb to 8b27b84 Compare March 21, 2018 18:53
@bukzor
Copy link
Contributor Author

bukzor commented Mar 21, 2018

I'm not 100% happy with the result, but brew style is happy now.

@MikeMcQuaid MikeMcQuaid merged commit 8307347 into Homebrew:master Mar 22, 2018
@MikeMcQuaid
Copy link
Member

Thanks again @bukzor!

@bukzor
Copy link
Contributor Author

bukzor commented Mar 22, 2018

Thank you too @MikeMcQuaid. It's a pleasure to work with you all. You and the rest of the homebrew team are a role model for open-source upstreams, and collaborative software in general.

@bukzor bukzor deleted the clang-build-version branch March 22, 2018 16:54
@MikeMcQuaid
Copy link
Member

@bukzor Thanks for the kind words and taking the time to say them. We end up dealing with a lot of bile so comments like this are touching ❤️

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants