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

Add support for building with Clang 19 #109198

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

MichaelSimons
Copy link
Member

Backport of #105141 to release/8.0
Upstreaming .NET 8.0 Fedora 41 patch

This is required so that source-build can build/test on Fedora 41 (dotnet/source-build#4593).

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 24, 2024
@MichaelSimons MichaelSimons requested review from omajid and am11 October 24, 2024 21:07
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you! 👍

@am11 am11 added area-Infrastructure-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 24, 2024
Copy link
Contributor

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

@omajid
Copy link
Member

omajid commented Oct 25, 2024

I forgot to note it down in my original Fedora patch, but the changes to src/native/libs/CMakeLists.txt are from a different PR: #101871

@omajid
Copy link
Member

omajid commented Oct 28, 2024

cc @dviererbe who had to use a similar fix on Ubuntu

@dviererbe
Copy link
Contributor

cc @dviererbe who had to use a similar fix on Ubuntu

Correct, we have a patch for Ubuntu >= 24.10. Ubuntu 24.04 uses clang 18 and Ubuntu 22.04 uses clang 14 and therefore does not require the patch.

Nice that this gets backported :)

@dviererbe
Copy link
Contributor

We also have a patch in place for clang 18. Shouldn't this be backported first before adding clang 19 support?

@MichaelSimons
Copy link
Member Author

We also have a patch in place for clang 18. Shouldn't this be backported first before adding clang 19 support?

You should feel free to open a PR to upstream your patch. I'm not seeing a reason to order the changes.

@MichaelSimons
Copy link
Member Author

@jkoritzinsky, what is the runtime's process for merging infra changes like this? Does this require tactics approval or tell mode?

@jkoritzinsky
Copy link
Member

We usually do tell-mode for infra-only changes. I'm currently OOF, @hoyosjs can you help get this in?

@hoyosjs
Copy link
Member

hoyosjs commented Nov 15, 2024

Is this release 8 only?

@hoyosjs hoyosjs changed the base branch from release/8.0 to release/8.0-staging November 15, 2024 07:01
@hoyosjs hoyosjs added the Servicing-consider Issue for next servicing release review label Nov 15, 2024
@hoyosjs
Copy link
Member

hoyosjs commented Nov 15, 2024

Changing to staging branch seems to have messed this. I'll fix this tomorrow.

@am11
Copy link
Member

am11 commented Nov 15, 2024

Changing to staging branch seems to have messed this. I'll fix this tomorrow.

Usually infra (build related) changes are directly pushed to the release branch (#107921), while product changes go through staging. It's a bit confusing where we draw the line.

@hoyosjs
Copy link
Member

hoyosjs commented Nov 15, 2024

That's usually only true if there's an undergoing release build and the error is seen after merge of staging, since there won't be more merges. That PR you mention was retargeted to the staging branch.

MichaelSimons and others added 2 commits November 15, 2024 11:40
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we can take this as tell mode

cc @carlossanlop

@MichaelSimons
Copy link
Member Author

Can someone help get this merged in? Source-build CI needs this fix.

@jkoritzinsky
Copy link
Member

/ba-g Failures are unrelated

@jkoritzinsky jkoritzinsky merged commit b2ac274 into dotnet:release/8.0-staging Dec 3, 2024
169 of 174 checks passed
@MichaelSimons MichaelSimons deleted the clang19 branch December 3, 2024 20:38
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants