Conversation
…s without needing to change builder
…me in case things change
| # - Supports any future version (8+) dynamically | ||
| # - Removes the need for clangd, lldb | ||
|
|
||
| LLVM_SH = """\ |
There was a problem hiding this comment.
Why do we do this rather than a separate file?
There was a problem hiding this comment.
Unsure, I touched as little of the existing framework as I could because I didn't want to break anything else. A lot of this felt pretty hacky. If you mean the entire implementation as a whole, it does still feel like the changes should be in this file since we're parsing the arguments for clang here.
There was a problem hiding this comment.
have you tried the latest https://apt.llvm.org/llvm.sh?
Look into this, it seems to be outdated comparing to what I just downloaded.
Does the latest script supports future version (8+)?
There was a problem hiding this comment.
Two immediate problems are that the latest doesn't have immediate support for anything lower than clang-9 (so we'd need to hardcode earlier versions) and it has its version hardcoded up to 21 so later versions as things change will need to be manually addressed which is what I'm trying avoid doing here. We might be able to use this as a NEW base to work off but I don't see a strong reason why we should.
| """ Finds clang, clang-tidy, lld, etc at a specific version, or the | ||
| latest one available """ | ||
| versions = [version] if version else _clang_versions() | ||
| latest one available. If version is 'latest', resolves it dynamically. """ |
There was a problem hiding this comment.
So, we are trying to resolve the latest one available when the version is not set, but we are also introducing a new latest version?
Sounds like we should just resolve the latest version dynamically when the version is None instead of the latest one in the hard-coded list?
There was a problem hiding this comment.
We introduce latest to be explicit about what we want to do when calling from CI; find the latest version of clang that can be used from llvm. None simply asks to use any available version and doesn't go through the process of looking for latest from llvm. I think we should keep this separation in behavior.
3 participants
Allow CI to specify
latest-clangand dynamically check llvm for its latest qualification version of clang to build against.When
latestis used, we will parse for the latest qualification branch from llvm and use that. Sometimes the latest version that is listed (whether qualification or stable) isn't actually available for the specific codename we are using so we do a check in order of versions we want before selecting and using it.Also removed hardcoding of versions past 8 to be dynamically set so that we don't need to continually update builder for later version support. Left comments in case it breaks and we need to return to hardcoding but that shouldn't be necessary.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.