Uh oh!
There was an error while loading. Please reload this page.
MLDsaOpenSsl + tests - #114485
Conversation
ghost
commented
Apr 10, 2025
Note regarding the |
1 similar comment
ghost
commented
Apr 10, 2025
Note regarding the |
There was a problem hiding this comment.
Copilot reviewed 15 out of 18 changed files in this pull request and generated no comments.
Files not reviewed (3)
- src/libraries/System.Security.Cryptography/src/Resources/Strings.resx: Language not supported
- src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported
- src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj: Language not supported
Comments suppressed due to low confidence (1)
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.h:8
- Consider aligning the native enum name with the managed interop naming (e.g. 'PalMLDsaAlgorithmId' instead of 'PalMLDsaId') to improve clarity and reduce potential confusion between layers.
typedef enum
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bartonjs
commented
Apr 10, 2025
#114471 is adding a few TODOs regarding MLDsaOpenSsl's future existence. Assuming it's merged before you iterate on this, consider trying to address them in this PR. But if that feels like too much scope creep, it can be a followup. |
vcsjones
commented
Apr 10, 2025
Sorry. That was a fat-finger :-/ |
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bartonjs
commented
Apr 12, 2025
@vcsjones I don't see a way for me to trigger a 3.5 run. Would you mind? |
PranavSenthilnathan
commented
Apr 12, 2025
@vcsjones-bot test 727a0bc with openssl-3.5 |
1 similar comment
vcsjones
commented
Apr 12, 2025
@vcsjones-bot test 727a0bc with openssl-3.5 |
vcsjones
commented
Apr 12, 2025
The bot ignores everyone except for me, for now. Will work on that later. |
vcsjones
commented
Apr 12, 2025
The results will be here https://github.com/vcsjones/runtime-ci/actions/runs/14414503783 |
bartonjs
commented
Apr 12, 2025
Looks like the new tests are all happy in https://github.com/vcsjones/runtime-ci/actions/runs/14414503783. |
krwq
commented
Apr 15, 2025
failures look unrelated, merging |
MichalStrehovsky
commented
Apr 17, 2025
@krwq I believe this is breaking native AOT outerloop runs on all linux flavors with: Searching through the repo, I see we have a LibraryImport for these APIs, but I can't seem to find an implementation. It's not clear to me why it's not failing on CoreCLR - the only plausible explanation would be that there's no test coverage of these APIs and the codepath is not exercised at runtime. CoreCLR fails just in time for things like this, native AOT fails ahead of time. Could you please have a look today or revert if the investigation takes longer? |
krwq
commented
Apr 17, 2025
@MichalStrehovsky I'll take a look |
PranavSenthilnathan
commented
Apr 17, 2025
@krwq Those methods are not used so they can be safely removed. I have the changes in https://github.com/dotnet/runtime/pull/114060/files#diff-083ed7205b5ecd53c32df803bd31400a901aba8981fd1b71830817a81ca61378 already but feel free to push them yourself if you don't want to wait for that. |
krwq
commented
Apr 17, 2025
@PranavSenthilnathan if you're planning on merging today then I'll wait, otherwise will send targeted PR tomorrow if this is not addressed yet |
Contributes to: #113502