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.

Sign up for GitHub

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

Jump to bottom

[release/7.0] Suppress clang-16 warnings (backport #81573) #84444

Merged
merged 2 commits into from
May 15, 2023
Merged

[release/7.0] Suppress clang-16 warnings (backport #81573) #84444

merged 2 commits into from
May 15, 2023

Conversation

ayakael
Copy link
Contributor

@ayakael ayakael commented Apr 6, 2023 •
edited
Loading

Backports #81573 and #82461

Customer impact

Without this, .NET 7 fails to build on platforms that use clang-16

Testing

Tested on CI and manually on Alpine Linux edge.

Risk

Low. The flag that is set makes the compiler less strict.

@am11 @omajid ptal.

@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 Apr 6, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 6, 2023
@ayakael ayakael changed the title [release/7.0] Suppress-clang-16 warnings [release/7.0] Suppress clang-16 warnings Apr 6, 2023
@ayakael ayakael changed the title [release/7.0] Suppress clang-16 warnings [release/7.0] Suppress clang-16 warnings (backport #81573) Apr 6, 2023
@am11
Copy link
Member

am11 commented Apr 6, 2023

It needs the DBI fix: #82461.

@ayakael ayakael force-pushed the backport/pr-81573-to-release/7.0 branch 2 times, most recently from 60dfa30 to b5b3114 Compare April 6, 2023 20:38
@ayakael ayakael mentioned this pull request Apr 6, 2023
Merged
@ayakael
Copy link
Contributor Author

ayakael commented Apr 6, 2023 •
edited
Loading

It needs the DBI fix: #82461.

Done :)

@ayakael ayakael closed this Apr 6, 2023
@ayakael ayakael reopened this Apr 6, 2023
@am11
Copy link
Member

am11 commented Apr 6, 2023 

- Backports https://github.com/dotnet/runtime/pull/81573
+ Backports https://github.com/dotnet/runtime/pull/81573 and https://github.com/dotnet/runtime/pull/82461

Otherwise, LGTM. 👍

cc @mikem8361

@am11
Copy link
Member

am11 commented Apr 6, 2023

The backport workflow has changed. I think this PR should target release/7.0-staging and #84443 should target release/6.0-staging.

See:

runtime/.github/workflows/backport.yml

Line 33 in 531bf3c

- The PR target branch is `release/X.0-staging`, not `release/X.0`.

@ayakael ayakael force-pushed the backport/pr-81573-to-release/7.0 branch from 7cce9c8 to 231f88f Compare April 6, 2023 21:08
@ayakael ayakael changed the base branch from release/7.0 to release/7.0-staging April 6, 2023 21:08
@ayakael ayakael changed the title [release/7.0] Suppress clang-16 warnings (backport #81573) [release/7.0-staging] Suppress clang-16 warnings (backport #81573) Apr 6, 2023
@ayakael ayakael changed the title [release/7.0-staging] Suppress clang-16 warnings (backport #81573) [release/7.0] Suppress clang-16 warnings (backport #81573) Apr 6, 2023
@carlossanlop
Copy link
Member

@jeffschwMSFT do you know who can help champion this? Today is code complete for the May release. If we want to include this change in that release, we would need it to be ready to merge by 4pm PT today:

  • Approved by Tactics if this is ask-mode.
  • CI failures determined as unrelated.
  • Signed-off by an area owner.
  • No OOB changes needed since it's not affecting managed libraries code.

@jeffschwMSFT
Copy link
Member

@tommcdon can you take a look and request folks to code review? Let's plan on having ready for review in tactics on Tue.

@tommcdon tommcdon requested review from mikem8361 and hoyosjs April 10, 2023 17:55
@carlossanlop carlossanlop mentioned this pull request Apr 10, 2023
Merged
hoyosjs
hoyosjs approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Other than that, LGTM

eng/native/configurecompiler.cmake
# other clang 16.0 suppressions
add_compile_options(-Wno-single-bit-bitfield-constant-conversion)
add_compile_options(-Wno-cast-function-type-strict)
add_compile_options(-Wno-incompatible-function-pointer-types-strict)
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what needs this? I can't seem to find this needed in any other place. I also didn't see it on lld's man page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came up when trying to build on Alpine Linux with clang 16. I reported it in #81577 to not forget to investigate.

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Apr 10, 2023
@jeffschwMSFT jeffschwMSFT added this to the 7.0.x milestone Apr 10, 2023
@jeffschwMSFT
Copy link
Member

do we understand the ci failures?

@hoyosjs
Copy link
Member

hoyosjs commented Apr 10, 2023

A fair amount of them are #80619. Analysis is hard because of dotnet/arcade#13066

@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Apr 10, 2023
Merged
@tommcdon tommcdon requested a review from agocke April 10, 2023 21:48
@tommcdon
Copy link
Member

Adding @agocke for the nativeaot related changes

@am11
Copy link
Member

am11 commented Apr 11, 2023

Adding @agocke for the nativeaot related changes

There is no NativeAOT change in this PR. :)

@tommcdon
Copy link
Member

Adding @agocke for the nativeaot related changes

There is no NativeAOT change in this PR. :)

Ahh yes, thanks for pointing that out @am11! I meant native corehost, not native aot :).

@danmoseley danmoseley added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Backports #81573 and #82461

Customer impact

Without this, .NET 7 fails to build on platforms that use clang-16

Testing

Tested on CI and manually on Alpine Linux edge.

Risk

Low. The flag that is set makes the compiler less strict.

@am11 @omajid ptal.

Author: ayakael
Assignees: -
Labels:

Servicing-consider, area-Infrastructure, community-contribution
Milestone: 7.0.x

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 11, 2023
@leecow leecow modified the milestones: 7.0.x, 7.0.7 Apr 11, 2023
@carlossanlop
Copy link
Member

Hey all, today is Code Complete for the June Release. If this good to merge, please help merge the PR. @agocke, @hoyosjs @mikem8361 .

@akoeplinger akoeplinger merged commit 6f9d91c into dotnet:release/7.0-staging May 15, 2023
@radical radical mentioned this pull request Jun 15, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@am11 am11 am11 left review comments

@hoyosjs hoyosjs hoyosjs approved these changes

@akoeplinger akoeplinger akoeplinger approved these changes

@radical radical Awaiting requested review from radical

@lewing lewing Awaiting requested review from lewing

@pavelsavara pavelsavara Awaiting requested review from pavelsavara

@vargaz vargaz Awaiting requested review from vargaz

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@thaystg thaystg Awaiting requested review from thaystg

@marek-safar marek-safar Awaiting requested review from marek-safar

@mikem8361 mikem8361 Awaiting requested review from mikem8361

@agocke agocke Awaiting requested review from agocke

Assignees
No one assigned
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Milestone
7.0.7
Development

Successfully merging this pull request may close these issues.
9 participants
@ayakael @am11 @carlossanlop @jeffschwMSFT @hoyosjs @tommcdon @akoeplinger @leecow @danmoseley