Update Source-Build SDK Diff Tests Baselines and Exclusions - #7754

I think this will get fixed by a backflow of #7736 to SBA release/10.0, which will then forward flow to VMR's release/10.0.4xx branch. @mthalman - does the flow logic sound right?

Yep, agreed.

I think this will get fixed by a backflow of #7736 to SBA release/10.0, which will then forward flow to VMR's release/10.0.4xx branch. @mthalman - does the flow logic sound right?

Yep, agreed.

@mthalman - looks like this flow never happened?? See https://github.com/dotnet/source-build-assets/tree/release/10.0/src

I think this will get fixed by a backflow of #7736 to SBA release/10.0, which will then forward flow to VMR's release/10.0.4xx branch. @mthalman - does the flow logic sound right?

Yep, agreed.

@mthalman - looks like this flow never happened?? See https://github.com/dotnet/source-build-assets/tree/release/10.0/src

There hasn't been a passing 10.0 build until this morning. I triggered the subscription flow and PR is ready: dotnet/source-build-assets#1751

Is the correct thing to do here to rerun - done at https://dev.azure.com/dnceng/internal/_build/results?buildId=3033550&view=results - the pipeline for baseline diff tests and close this PR?

Is it ok to merge the 10.0.3xx version? #7905 (I approved it before I saw this conversation.) I think that one might not get updates but 10.0.3xx is dead in the water post August so maybe it's not a big deal.

Is the correct thing to do here to rerun - done at https://dev.azure.com/dnceng/internal/_build/results?buildId=3033550&view=results - the pipeline for baseline diff tests and close this PR?

Is it ok to merge the 10.0.3xx version? #7905 (I approved it before I saw this conversation.) I think that one might not get updates but 10.0.3xx is dead in the water post August so maybe it's not a big deal.

That pipeline failed for the same reason as this one, so I think I need to re build the unified build, which I did here: https://dev.azure.com/dnceng/internal/_build/results?buildId=3033574

I think the next step is to rerun the other pipeline https://dev.azure.com/dnceng/internal/_build/results?buildId=3033550&view=results. Does that sound right @ellahathaway?

Is the correct thing to do here to rerun - done at https://dev.azure.com/dnceng/internal/_build/results?buildId=3033550&view=results - the pipeline for baseline diff tests and close this PR?
Is it ok to merge the 10.0.3xx version? #7905 (I approved it before I saw this conversation.) I think that one might not get updates but 10.0.3xx is dead in the water post August so maybe it's not a big deal.

That pipeline failed for the same reason as this one, so I think I need to re build the unified build, which I did here: https://dev.azure.com/dnceng/internal/_build/results?buildId=3033574

I think the next step is to rerun the other pipeline https://dev.azure.com/dnceng/internal/_build/results?buildId=3033550&view=results. Does that sound right @ellahathaway?

Since UB builds are large, we usually just wait until they get queued on their own.

https://dev.azure.com/dnceng/internal/_build/results?buildId=3033700&view=results if this succeeds, I think we can then close this PR

dotnet-user-jwts is still causing a baseline change on the new build - investigating

I see that #7989 merged at 9 am so I think the next step is to update and rerun?

Actually I think this PR should be closed now and then I should run CreateBaselineUpdatePR again to confirm the baseline diff no longer is needed, is this correct @ellahathaway ?

I instead trigerred a new run of source-build-sdk-diff-test https://dev.azure.com/dnceng/internal/_build/results?buildId=3034632&view=results which should hopefully update this PR and then the changes will be gone.

It looks like the baseline diff still shows the source-built dotnet-user-jwts  IdentityModel lags the ML‑DSA enabled build. I believe the run had the necessary changes since it was pinned off 9cfaa55c. @ellahathaway Should we keep these baseline diff updates or am I missing something and they aren't needed still?

The Microsoft-built SDK and source-built SDK intentionally build different variants of the IdentityModel package used by dotnet-user-jwts.

The Microsoft-built SDK uses IdentityModel 8.19.2 with its ML-DSA implementation enabled. This introduces:

• A dependency on Microsoft.Bcl.Cryptography
• Microsoft.Bcl.Cryptography.dll in the dotnet-user-jwts directory
• New ML-DSA APIs in Microsoft.IdentityModel.Tokens.dll

Source-build also starts from IdentityModel 8.19.2, but applies the Revert 10.0 BCL.Crypto update patch. That patch removes the Microsoft.Bcl.Cryptography dependency and the corresponding ML-DSA APIs. Therefore, the source-built dotnet-user-jwts does not contain Microsoft.Bcl.Cryptography.dll.

So the diff is expected. I've logged dotnet/source-build#5626 to resolve this diff and remove the patch. In the meantime, an exclusion should be added for the Microsoft.Bcl.Cryptography path instead of including it in the baseline. That way we can include a comment that references dotnet/source-build#5626.

The Microsoft-built SDK and source-built SDK intentionally build different variants of the IdentityModel package used by dotnet-user-jwts.

The Microsoft-built SDK uses IdentityModel 8.19.2 with its ML-DSA implementation enabled. This introduces:

• A dependency on Microsoft.Bcl.Cryptography
• Microsoft.Bcl.Cryptography.dll in the dotnet-user-jwts directory
• New ML-DSA APIs in Microsoft.IdentityModel.Tokens.dll

Source-build also starts from IdentityModel 8.19.2, but applies the Revert 10.0 BCL.Crypto update patch. That patch removes the Microsoft.Bcl.Cryptography dependency and the corresponding ML-DSA APIs. Therefore, the source-built dotnet-user-jwts does not contain Microsoft.Bcl.Cryptography.dll.

So the diff is expected. I've logged dotnet/source-build#5626 to resolve this diff and remove the patch. In the meantime, an exclusion should be added for the Microsoft.Bcl.Cryptography path instead of including it in the baseline. That way we can include a comment that references dotnet/source-build#5626.

Thanks @mthalman for helping, I'm happy to approve this to move forward but I don't really understand source build enough to understand whether this is the right change or not.

Broad Questions:
For example, how does the source built SDK work at all if we don't have Microsoft.Bcl.Cryptography.dll - is the library pulled in or replaced by dotnet-user-jwts? Is dotnet-user-jwts simply including a source code copy of the BCL library that can be built using arcade / the dotnet build system (or some generic build system for distro partners)? Another question I have is that Microsoft.Bcl.Cryptography seems to be open source, so I don't see why we would need to do that, because it seems like building that source could be done without that, but maybe their code is hard to build or has a different build system so to unify the build system we pull it in here?

Specific Questions:
Earlier in the thread we agreed that there was a new dependency, Microsoft.Bcl.Cryptography.dll - was the expectation here that this would flow in and be part of source build? I thought the patch or other changes to the exclusions would happen automatically when the code flowed in and the baseline test change would nullify automatically? I don't understand why we need this patch on top of the exclusion chnage here: https://github.com/dotnet/source-build-assets/blob/07e5dbddc0154145ed167c08fdedcc64e0e5613b/src/externalPackages/patches/azure-activedirectory-identitymodel-extensions-for-dotnet/0001-Revert-10.0-BCL.Crypto-update.patch

For example, how does the source built SDK work at all if we don't have Microsoft.Bcl.Cryptography.dll - is the library pulled in or replaced by dotnet-user-jwts? Is dotnet-user-jwts simply including a source code copy of the BCL library that can be built using arcade / the dotnet build system (or some generic build system for distro partners)?

The patch removes the reference and source dependencies that the code had on Microsoft.Bcl.Cryptography.

Another question I have is that Microsoft.Bcl.Cryptography seems to be open source, so I don't see why we would need to do that, because it seems like building that source could be done without that, but maybe their code is hard to build or has a different build system so to unify the build system we pull it in here?

It is built from source. But as IdentityModel repo was defined when updating our submodule, it was causing prebuilts to occur by having a dependency on a version of Microsoft.Bcl.Cryptography that hadn't been built yet (source-build-assets repo is built before runtime repo, where Microsoft.Bcl.Cryptography is defined).

Specific Questions: Earlier in the thread we agreed that there was a new dependency, Microsoft.Bcl.Cryptography.dll - was the expectation here that this would flow in and be part of source build?

The understanding of things earlier in the thread was incorrect. We had though the updates from SBA would fix things but they actually don't because the patch removes the dependency on Crypto.

I thought the patch or other changes to the exclusions would happen automatically when the code flowed in and the baseline test change would nullify automatically?

The patch is only being applied to source-build, not the Microsoft build. So the version of the IdentityModel package that the Microsoft build uses has the dependency while source-build does not.

I don't understand why we need this patch on top of the exclusion chnage here: https://github.com/dotnet/source-build-assets/blob/07e5dbddc0154145ed167c08fdedcc64e0e5613b/src/externalPackages/patches/azure-activedirectory-identitymodel-extensions-for-dotnet/0001-Revert-10.0-BCL.Crypto-update.patch

See my explanation above.

Thank you for that explanation and for taking the time to answer my questions. I have new broader questions about source-build, but I think I understand what is happening now with these changes and why we want them.