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

Fix CI #470

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Fix CI #470

merged 1 commit into from
Jul 15, 2022

Conversation

dabrahams
Copy link
Contributor

@dabrahams dabrahams commented Jul 15, 2022

Also makes our claims of testing with Apple clang not-a-lie.

@dabrahams
Copy link
Contributor Author

Fixes #467

Copy link
Contributor

@camio camio left a comment

Choose a reason for hiding this comment

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

Looks good. Could you also please update

"name": "macOS apple-clang 13.0.1",
with whatever version number we're using now? Based on this it looks like this results in a switch to 13.0.0.

@dabrahams
Copy link
Contributor Author

Could you also please update

"name": "macOS apple-clang 13.0.1",

with whatever version number we're using now?

IMO that should happen after #472; otherwise we're just guessing.

@dabrahams
Copy link
Contributor Author

@camio Please note also #471
Isn't it more important to get CI working again, so we can start shoring up this mare's nest properly?

@camio
Copy link
Contributor

camio commented Jul 15, 2022

I'm assuming that the specification I linked to is correct. I have no reason to believe otherwise.

Also Clang/LLVM 13.1.6 is available, but using the macOS 12.4 image as specified here.

I'd rather a new inconsistency not be introduced in a PR. IMO we generally shouldn't knowingly introduce tech debt in a PR.

@dabrahams
Copy link
Contributor Author

I'd rather a new inconsistency not be introduced in a PR. IMO we generally shouldn't knowingly introduce tech debt in a PR.

IMO CI being broken is an emergency, and this is a priority inversion. With the change it is no more inconsistent than it was. It said "apple clang 13.0.1" but that's not what we were testing; it was “stock clang 13.0.1”. This PR is clearly forward progress, and PR review shouldn't stand in the way of that. Sorry, I'm making the call and merging. See #473.

@dabrahams dabrahams merged commit 92dd592 into stlab:main Jul 15, 2022
camio added a commit to camio/stlab-libraries that referenced this pull request Jul 15, 2022
camio added a commit that referenced this pull request Jul 16, 2022
@dabrahams
Copy link
Contributor Author

dabrahams commented Oct 11, 2022 via email

@camio
Copy link
Contributor

camio commented Oct 12, 2022 via email

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.

2 participants