Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop trying to be clever when finding user's LLVM installation #3077

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

SeanTAllen
Copy link
Member

Clever is bad. Eventually it bites you in the ass. Ask The Narrator, he
knows.

Prior to this commit, we were trying to be clever and helpful and would
bend over backwards to try and find the user's LLVM installation
whereever their package manager of choice might put it. This was, in the
end, a losing battle. We were constantly adding more and more
variations.

To make it worse, we tried to have a hierarchy where we would look for
LLVM 7 first then 6 etc. We did this by looking something like:

llvm-config-7
llvm-config-6

The problem with this approach was that some installations, wouldn't use
the -7 notation. For example, if you download a release directly from
releases.llvm.org, then your llvm-config will simply be llvm-config.

I discovered this when, after installing LLVM 7 from releases.llvm.org,
I continued to have LLVM 6 used because the search loooked something
like:

llvm-config-7
llvm-config-6
... more stuff ...
llvm-config

In the end, that's all just too much clever.

With this change, we'll use the first llvm-config we find in your path
IF you haven't set the LLVM_CONFIG environment variable which should be
considered the preferred way to select an LLVM when building ponyc.

Closes #3072

@SeanTAllen SeanTAllen requested a review from jemc March 8, 2019 01:48
@SeanTAllen SeanTAllen added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Mar 8, 2019
Clever is bad. Eventually it bites you in the ass. Ask The Narrator, he
knows.

Prior to this commit, we were trying to be clever and helpful and would
bend over backwards to try and find the user's LLVM installation
whereever their package manager of choice might put it. This was, in the
end, a losing battle. We were constantly adding more and more
variations.

To make it worse, we tried to have a hierarchy where we would look for
LLVM 7 first then 6 etc. We did this by looking something like:

llvm-config-7
llvm-config-6

The problem with this approach was that some installations, wouldn't use
the `-7` notation. For example, if you download a release directly from
releases.llvm.org, then your llvm-config will simply be `llvm-config`.

I discovered this when, after installing LLVM 7 from releases.llvm.org,
I continued to have LLVM 6 used because the search loooked something
like:

llvm-config-7
llvm-config-6
... more stuff ...
llvm-config

In the end, that's all just too much clever.

With this change, we'll use the first `llvm-config` we find in your path
IF you haven't set the LLVM_CONFIG environment variable which should be
considered the preferred way to select an LLVM when building ponyc.

Closes #3072
@jemc
Copy link
Member

jemc commented Mar 8, 2019

Are there any updates we should make to the README to indicate that setting LLVM_CONFIG is the preferred way to indicate the LLVM version?

@SeanTAllen
Copy link
Member Author

I was thinking that if people bring it up beyond what is in the new error message, that we could update as needed.

@SeanTAllen SeanTAllen merged commit f432dbb into master Mar 8, 2019
@SeanTAllen SeanTAllen deleted the issue-3072 branch March 8, 2019 16:04
ponylang-main added a commit that referenced this pull request Mar 8, 2019
SeanTAllen pushed a commit that referenced this pull request Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants