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

Revert Microsoft.CodeAnalysis package changes in prebuilt baseline #48212

Merged
merged 2 commits into from
May 12, 2023

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented May 12, 2023

The changes that were made to the prebuilt baseline in #48179 can be reverted now that SBRPs for the offending packages have been defined in dotnet/source-build-reference-packages#666 and been flowed into aspnetcore.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 12, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 12, 2023
@ghost
Copy link

ghost commented May 12, 2023

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@mthalman mthalman changed the title Remove Microsoft.CodeAnalysis packages from allowed prebuilts Revert Microsoft.CodeAnalysis package changes in prebuilt baseline May 12, 2023
@mthalman mthalman marked this pull request as ready for review May 12, 2023 13:44
@mthalman mthalman requested review from a team and wtgodbe as code owners May 12, 2023 13:44
@mthalman mthalman requested a review from eerhardt May 12, 2023 13:44
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

I just had one question about the CodeAnalysis versions that our analyzers are using.

Comment on lines +16 to +19
<UsagePattern IdentityGlob="Microsoft.CodeAnalysis.Common/*3.3.1*" />
<UsagePattern IdentityGlob="Microsoft.CodeAnalysis.CSharp/*3.3.1*" />
<UsagePattern IdentityGlob="Microsoft.CodeAnalysis.CSharp.Workspaces/*3.3.1*" />
<UsagePattern IdentityGlob="Microsoft.CodeAnalysis.Workspaces.Common/*3.3.1*" />
Copy link
Member

Choose a reason for hiding this comment

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

Should these 3.3.1 packages also be in SBRP?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to @mmitche, these are executed at build time so we don't want them in SBRP since those are reference implementation only.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I think we may be getting into a corner, eventually. We may end up with packages that should be SBRPs for some repos, and not SBRPs for others. @dotnet/source-build-internal Has this happened before?

Copy link
Member

Choose a reason for hiding this comment

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

We may end up with packages that should be SBRPs for some repos, and not SBRPs for others.

We have had the case where a reference package satisfies on repo's requirements but not another because it is loaded. In those cases the repo that loads the assembly would need to use a live/current version.

@captainsafia
Copy link
Member

@mthalman This PR looks good to me. We plan on upgrading our M.CA dependencies soon in order to complete some feature work and I want to get a sense of what the process should be in order to play nice with source build. If we want to upgrade to M.CA to 4.6.0 we would need to:

  1. Add definitions for the packages in dotnet/source-build-reference-packages
  2. Wait for dependency flow from dotnet/source-build-reference-packages to merge to aspnetcore
  3. Upgrade the dependency version in aspnetcore
  4. ???

For 4, do we need to make any changes in the SDK?

@mthalman
Copy link
Member Author

@mthalman This PR looks good to me. We plan on upgrading our M.CA dependencies soon in order to complete some feature work and I want to get a sense of what the process should be in order to play nice with source build. If we want to upgrade to M.CA to 4.6.0 we would need to:

  1. Add definitions for the packages in dotnet/source-build-reference-packages
  2. Wait for dependency flow from dotnet/source-build-reference-packages to merge to aspnetcore
  3. Upgrade the dependency version in aspnetcore
  4. ???

For 4, do we need to make any changes in the SDK?

I would reverse the order a bit:

  1. Make the PR to upgrade the dependency version
  2. If the PR build results in SB prebuilts being detected:
    a. Add definitions for the packages in dotnet/source-build-reference-packages
    b. Wait for dependency flow from dotnet/source-build-reference-packages to merge to aspnetcore
  3. Continue with PR from step 1

I don't think any changes would be necessary in installer.

@mthalman mthalman merged commit e4d2f43 into dotnet:main May 12, 2023
@mthalman mthalman deleted the prebuilt-cleanup branch May 12, 2023 17:36
@ghost ghost added this to the 8.0-preview5 milestone May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants