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

CI run-tests script now attempts to detect Apple Clang 15 and other unsupported regression-tests compiler versions #972

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

bluetarpmedia
Copy link
Contributor

@bluetarpmedia bluetarpmedia commented Feb 7, 2024

After investigating some spurious macOS regression-test job failures, I found that the macos-13 GitHub runner has both Apple Clang 14 and 15 installed.

It appears that on some runs, running clang++ incorrectly selects Apple Clang 15, which is currently unsupported as a compiler version for the regression-tests. For example, see this log file for this failed job.

This PR makes 2 changes:

  • If Apple Clang 15 is detected it tries to switch to 14 instead
  • Prints an error message and aborts the job if an unsupported compiler version is found

CC @jarzec

…rt it

The `macos-13` runner has both Apple Clang 14 and 15 installed, and some CI workflow runs appear to use 15, causing spurious failures.
@gregmarr
Copy link
Contributor

gregmarr commented Feb 7, 2024

Is there a reason to not add support for Clang 15?

@hsutter
Copy link
Owner

hsutter commented Feb 7, 2024

incorrectly selects Apple Clang 15, which is currently unsupported as a compiler version for the regression-tests

Is there an issue with Apple Clang 15, or just that there's no subdirectory for Apple Clang 15 in the test results?

@jarzec
Copy link
Contributor

jarzec commented Feb 7, 2024

@hsutter In fact I wanted to propose adding support for Clang 15. I think the run-test.sh script has some leftovers of my trials for that. This version is available on Ubuntu 2204 and is installed by default on the GitHub runner. This is also the case on the macos-13 runner. The added value of adding this version is that all regression tests can be compiled and run using it, so it would be the only compiler on POSIX systems allowing for that (at least that was the case last time I tried). Can quickly prepare a PR with that.

compiler_version=$("$cxx_compiler" --version)

# We don't currently support Apple Clang 15 so try and switch to 14
if [[ "$compiler_version" == *"Apple clang version 15.0"* ]]; then
Copy link
Contributor

@jarzec jarzec Feb 7, 2024

Choose a reason for hiding this comment

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

[Question] Are you sure Apple Clang 15 was selected? The GitHub runner states that Clang 15 s installed from Homebrew. I checked and the output of $(brew --prefix llvm@15)/bin/clang --version is:

Homebrew clang version 15.0.7
Target: x86_64-apple-darwin22.6.0
Thread model: posix
InstalledDir: /usr/local/opt/llvm@15/bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'm not 100% sure, this is very much some investigative work!

All I can say for certain right now is that on some macOS job runs (e.g. the one linked above, and I've seen it other times too), clang++ does not resolve to Apple Clang 14. That's why the script produces the not found for compiler: 'clang++' error message.

One of the updates in this PR is to provide more info about why the job fails. If this PR is merged then in the future we'll get more info due to this:

printf "Unhandled compiler version:\n$compiler_version\n\n"

That may show Apple clang 15.0 etc or something else.

Yeah, the macos-13 runner confusingly has both LLVM Clang installed as well as multiple versions of Xcode (with Apple Clang).

image

Xcode 14.3 is meant to be the default so clang++ would resolve to Apple Clang 14, but I suspect there's a problem which is causing Xcode 15 to be active. But, it could also somehow be invoking LLVM Clang 15. To me that seemed less likely because, like you noted, it's installed via Homebrew and needs the brew --prefix llvm@15 to get the correct path.

@bluetarpmedia
Copy link
Contributor Author

Is there a reason to not add support for Clang 15?

Nope, that's definitely an option. (Note there's Clang 15 as well as Apple Clang 15.)

@bluetarpmedia
Copy link
Contributor Author

Is there an issue with Apple Clang 15, or just that there's no subdirectory for Apple Clang 15 in the test results?

Yeah, the latter, because there are no expected test-results yet for Apple Clang 15.

Even if we add test-results for Clang 15 and Apple Clang 15, I think it's still worth adding this part of the PR:

printf "Unhandled compiler version:\n$compiler_version\n\n"

That way in the future we'll get more details when the job fails due to a new compiler on the GitHub runner that we don't yet have test-results for.

@hsutter
Copy link
Owner

hsutter commented Feb 8, 2024

@jarzec I see an "approved these changes" from you midway up above, just checking: Does this look ready to merge to you?

@jarzec
Copy link
Contributor

jarzec commented Feb 8, 2024

@jarzec I see an "approved these changes" from you midway up above, just checking: Does this look ready to merge to you?

Yes, please do merge. I've started to look into including Clang 15 on top of the updates here.
(Sorry, I previously responded from my work account).

@hsutter hsutter merged commit 56f550d into hsutter:main Feb 9, 2024
15 checks passed
@hsutter
Copy link
Owner

hsutter commented Feb 9, 2024

Thanks!

@bluetarpmedia bluetarpmedia deleted the ci-detect-invalid-compiler branch May 16, 2024 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants