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 a direct reference to asn1 #8246

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

marcpopMSFT
Copy link
Member

in the most shared project to resolve the CG alerts. This is because pkcs depends on a version of this package that's not compliant but hasn't re-released a new version. That's pulled in by msbuild but the easiest way to fix is just add the direct reference.

@marcpopMSFT marcpopMSFT requested review from GangWang01 and a team as code owners July 26, 2024 21:22
@marcpopMSFT
Copy link
Member Author

@ericstj do you know what the guidance will be on transitive packages like this? It's pulled in by MSBuild that has a 6.0.0 dependency. Should I update this to an 8.0.1 version instead of 6.0.1 or just update the minimum amount to avoid the CG alert?

@ericstj
Copy link
Member

ericstj commented Jul 31, 2024

I think best practice would be CPM + Transitive Pinning. Then you just need to add a package to the list if it shows up in CG.

Barring that, I usually reccomend that folks do the following:

  1. Identify where the out of date dependency lives.
  2. If you own the source, just update it. If not, then see if there's an update to the package that references it.
  3. If not, look to the parent reference. Go to step 2. Continue until you find an update or reach a source you own.

In this way you minimize the churn, and place the update next to the dependency you are updating.

That said - the whole MSBuild issue is a different sort of problem. There are so many components that reference MSBuild but aren't responsible for deploying it. We really need a better story for plugin / host relationships like this. @baronfel and others are looking at making a way to reference hosts like this without leaking the servicing requirements for the host's dependencies.

@marcpopMSFT
Copy link
Member Author

@ericstj sorry if I wasn't clear. I was specifically asking if I should be updating to 6.0.1 since MSBuild depends on 6.0.0 or should I hoist the version all the way up to an 8.0.1 version? Templating wasn't onboarded to CPM until 9. Basically, if we had CPM should I bring in a new major version of the package or just do a smaller version upgrade?

@marcpopMSFT
Copy link
Member Author

@dotnet/source-build-internal we're going to need some good guidance around transitive dependencies like these and CG as tooling teams are going to want to add direct references (or CPM upgrades) which will break source build. I thought adding the maestro entry would have worked but it turns out that since I don't have any other runtime dependencies, the source build leg breaks.
/__w/1/s/.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.24360.5/tools/Tools.proj : error NU1101: Unable to find package Microsoft.SourceBuild.Intermediate.runtime. No packages exist with this id in source(s): dotnet-eng, dotnet-libraries, dotnet-public, dotnet-tools, dotnet6, dotnet6-transport, dotnet8, dotnet8-transport, nugetbuild #
Suggestions?

@MichaelSimons
Copy link
Member

" I thought adding the maestro entry would have worked but it turns out that since I don't have any other runtime dependencies, the source build leg breaks."

I think source build will work if you just update the source-build-reference-packages dependency to latest and drop the asn1 dependency all together. The asn1 dependency will get picked up as a reference package.

@MichaelSimons
Copy link
Member

There is an open dependency flow PR to update the SBRP dependency. Merge that and remove the System.Formats.Asn1 dependency that was added and I think everything will work.

@ericstj
Copy link
Member

ericstj commented Aug 1, 2024

@ericstj sorry if I wasn't clear. I was specifically asking if I should be updating to 6.0.1 since MSBuild depends on 6.0.0 or should I hoist the version all the way up to an 8.0.1 version? Templating wasn't onboarded to CPM until 9. Basically, if we had CPM should I bring in a new major version of the package or just do a smaller version upgrade?

I think these dependencies are actually coming from Nuget.Protocols and NuGet.Credentials, not MSBuild. The right answer is "it depends" who's actually shipping the binaries.

If it's this project then you can update and I'd say we should update to 8.0 since this is an 8.0 branch and we can keep things automatically up to date with dependency flow if we depend on 8.0.

If it's not this project then it's a harder call to make. It depends on what version you can count on the host shipping. For example - if it were VS you'd need to make sure the version you chose matched or was redirected to the one shipped in VS. Similar for SDK or MSBuild.

@marcpopMSFT
Copy link
Member Author

This project uses asn1 but I believe the SDK ships it. Looks like because SDK ships it, it ends up loading the 8.0 version so I should probably add that reference rather than 6.0.1. For some reason though, CG isn't flagging the SDK even though it's on 8.0.0 rather than 8.0.1

@marcpopMSFT
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Forgind
Copy link
Member

Forgind commented Aug 7, 2024

.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.24360.5/tools/SourceBuild/AfterSourceBuild.proj(68,5): error : (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) 1 new pre-builts discovered! Detailed usage report can be found at /__w/1/s/artifacts/source-build/self/prebuilt-report/baseline-comparison.xml.
See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
System.Formats.Asn1.8.0.1

I think we need to add that to SourceBuild @marcpopMSFT

@marcpopMSFT
Copy link
Member Author

.packages/microsoft.dotnet.arcade.sdk/8.0.0-beta.24360.5/tools/SourceBuild/AfterSourceBuild.proj(68,5): error : (NETCORE_ENGINEERING_TELEMETRY=AfterSourceBuild) 1 new pre-builts discovered! Detailed usage report can be found at /__w/1/s/artifacts/source-build/self/prebuilt-report/baseline-comparison.xml.
See https://aka.ms/dotnet/prebuilts for guidance on what pre-builts are and how to eliminate them.
Package IDs are:
System.Formats.Asn1.8.0.1

I think we need to add that to SourceBuild @marcpopMSFT

The externals package from SB that was recently merged may resolve this. I'm going to rerun and then ping the source build folks for a suggestion (as the other one of these they talked me out of an SBRP change)

@marcpopMSFT
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcpopMSFT
Copy link
Member Author

@dotnet/source-build-internal I've updated the version of Asn1 to 8.0.1 which shipped in July with the runtime. Do I need to add this to the reference packages? I had assumed that the source build updates post July would have pulled in the 8.0.1 version of this library. Suggestions?

@NikolaMilosavljevic
Copy link
Member

@dotnet/source-build-internal I've updated the version of Asn1 to 8.0.1 which shipped in July with the runtime. Do I need to add this to the reference packages? I had assumed that the source build updates post July would have pulled in the 8.0.1 version of this library. Suggestions?

@ellahathaway @mthalman @MichaelSimons what is the up to date guidance fir these types of changes? An exclusion in SourceBuildPrebuiltBaseline.xml is an option but perhaps it should be resolved differently. Thoughts?

@ellahathaway
Copy link
Member

@dotnet/source-build-internal I've updated the version of Asn1 to 8.0.1 which shipped in July with the runtime. Do I need to add this to the reference packages? I had assumed that the source build updates post July would have pulled in the 8.0.1 version of this library. Suggestions?

what is the up to date guidance fir these types of changes? An exclusion in SourceBuildPrebuiltBaseline.xml is an option but perhaps it should be resolved differently. Thoughts?

An entry for this package should be added to the Version.Details.xml file. This + the version prop in the Versions.props file will cause the dependency to get lifted to the live version while building the VMR (as long as this package is being produced as part of the sb build).

Assuming that no prebuilts exist while building the VMR with these changes, this package can be added to the exclusions baseline. I personally don't see a benefit to adding this package to SBRP if it'll get lifted to the live version in the VMR.

@marcpopMSFT
Copy link
Member Author

@ellahathaway so I should add the version.details.xml entry even though there is no codeflow from runtime set up. This will enable source build to find the right package and if there is an update in the future, I'll just have to manually update it, correct?

@ellahathaway
Copy link
Member

so I should add the version.details.xml entry even though there is no codeflow from runtime set up. This will enable source build to find the right package and if there is an update in the future, I'll just have to manually update it, correct?

Since there's no code flow, yes, you'll have to update this dependency manually in the future.

Otherwise, SB should now lift the dependency to the live version when building the VMR, but I'm going to test these changes locally to double-check that there's no prebuilts in the VMR. In the meantime, I'll defer to @MichaelSimons or @mthalman since they might have more info.

@marcpopMSFT
Copy link
Member Author

@ellahathaway now the build is failing to find the SB intermediate package. Do I need to add that to versions.details.xml as well now?

@ellahathaway
Copy link
Member

I spoke with @mthalman offline, and he indicated that the intermediate shouldn't be necessary in this case. I updated the version.details entry and added system.formats.asn1 to the exclusions baselines... hope that it's ok that I pushed those changes to your branch.

I also ported the changes to an 8.0 VMR test branch, and I'm testing the changes in this pipeline run (internal Microsoft link)

@marcpopMSFT marcpopMSFT merged commit b8bd284 into release/8.0.1xx Aug 14, 2024
10 checks passed
@ViktorHofer ViktorHofer deleted the marcpopMSTF-fixformsreference branch August 27, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants