Skip to content

Fix build break on linux-armel #115767

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

Merged
merged 4 commits into from
May 20, 2025
Merged

Fix build break on linux-armel #115767

merged 4 commits into from
May 20, 2025

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented May 20, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings May 20, 2025 08:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR suppresses unused-parameter warnings for DSA parameters on Linux ARMEL to fix a build break.

  • Adds void casts for hasSeed and hasSecretKey in CryptoNative_MLDsaGetPalId.

@jkotas
Copy link
Member Author

jkotas commented May 20, 2025

https://dev.azure.com/dnceng-public/public/_build/results?buildId=1046111&view=logs&jobId=2eda3965-9146-55d1-4226-c3d3f8432d8f&j=2eda3965-9146-55d1-4226-c3d3f8432d8f&t=27bbe0a6-6c05-550a-bc6f-305cf5eac8aa

  [ 10%] Building C object /__w/1/s/artifacts/obj/external/libunwind/CMakeFiles/libunwind.dir/__w/1/s/src/native/external/libunwind/src/arm/Linit_local.c.o
  /__w/1/s/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:10:85: error: unused parameter 'hasSeed' [-Werror,-Wunused-parameter]
     10 | int32_t CryptoNative_MLDsaGetPalId(const EVP_PKEY* pKey, int32_t* mldsaId, int32_t* hasSeed, int32_t* hasSecretKey)
        |                                                                                     ^
  /__w/1/s/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:10:103: error: unused parameter 'hasSecretKey' [-Werror,-Wunused-parameter]
     10 | int32_t CryptoNative_MLDsaGetPalId(const EVP_PKEY* pKey, int32_t* mldsaId, int32_t* hasSeed, int32_t* hasSecretKey)
        |                                                                                                       ^
  2 errors generated.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

@elinor-fung
Copy link
Member

The linux-armel leg didn't run in the original PR and is skipped in this one too. Should we be including src/native/libs/System.Security.Cryptography.Native/* in the paths for coreclr changes?

- subset: coreclr
include:
- src/libraries/System.Private.CoreLib/*
- src/native/libs/Common/*
- src/native/libs/System.Globalization.Native/*
- src/native/libs/System.IO.Compression.Native/*
exclude:

It is only used on non-Windows, so it might be broader than necessary - but I don't think our path evaluation makes that distinction.

@bartonjs
Copy link
Member

That might be the most expedient fix; but I think really it means that there should be a configuration group ("thing") that runs the libraries tests on "all relevant architectures" for any change under src/native/libs/.

The CLR tests aren't really relevant to this change... they just happen to be the only group that causes the armel build to run.

And, really, it's not so much the "run tests" as "build". So any change under src/native/ really seems like it should be "build everywhere".

@jkotas
Copy link
Member Author

jkotas commented May 20, 2025

That might be the most expedient fix; but I think really it means that there should be a configuration group ("thing") that runs the libraries tests on "all relevant architectures" for any change under src/native/libs/.

I think that the crux of the problem is that we have tiered validation system and that we do not expect to catch all breaks in the CI early. The question is whether catching this specific build break early is worth the extra CI cost, time and flakiness on many PRs.

I went back and forth on this. If we want to do something about this, it is best to do that in a separate PR.

@bartonjs
Copy link
Member

and flakiness

Yeah, that's why I walked it back to saying my interest is about "build", not "test".

The question is whether catching this specific build break early is worth the extra CI cost

I think it's rare enough that "no" is a fine answer. I'm more saying if something is done, it is about "build everywhere", and that if we think running tests are relevant we should run the relevant tests (in this case, it'd be a libraries concern, not a CLR concern).

If we want to do something about this, it is best to do that in a separate PR.

I agree.

@elinor-fung
Copy link
Member

The CLR tests aren't really relevant to this change... they just happen to be the only group that causes the armel build to run.

And, really, it's not so much the "run tests" as "build". So any change under src/native/ really seems like it should be "build everywhere".

Part of this is that the coreclr subset includes singlefilehost, which pulls in the src/native/libs - so in that sense, it is the build portion (I think the specific leg that would have hit this only builds). But yeah, if we do something here, we probably want a build vs test distinction.

If we want to do something about this, it is best to do that in a separate PR.

Agreed

@jkotas jkotas disabled auto-merge May 20, 2025 20:18
@jkotas jkotas merged commit afe0093 into dotnet:main May 20, 2025
93 of 98 checks passed
@jkotas jkotas deleted the build-break branch May 20, 2025 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants