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

Pin M.CA dependency versions used in RDG #48179

Merged
merged 4 commits into from
May 11, 2023
Merged

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented May 10, 2023

Resolves (🤞🏽) issues in dotnet/installer#16318

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label May 10, 2023
@captainsafia captainsafia added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework feature-rdg and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels May 10, 2023
@ghost
Copy link

ghost commented May 10, 2023

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

@wtgodbe
Copy link
Member

wtgodbe commented May 10, 2023

I prefer dotnet/installer#16318 (comment) if it's fast enough, it's more of a stage0 SDK issue than an aspnetcore issue (cc @mmitche)

@captainsafia
Copy link
Member Author

I prefer dotnet/installer#16318 (comment) if it's fast enough, it's more of a stage0 SDK issue than an aspnetcore issue (cc @mmitche)

I'd prefer both. I like the value that we get out of being able to control what version we build against in the event that this happens in the future.

@eerhardt
Copy link
Member

I also prefer both. I like that we get a consistent experience in and out of source-build. No surprises.

@eerhardt
Copy link
Member

Looks like you need to follow https://github.com/dotnet/source-build/blob/main/Documentation/eliminating-pre-builts.md#allowed-exceptions

dotnet/runtime's Regex source generator is already using this version (4.5.0), so it should be OK to use it in source-build.

FYI - @mthalman

@@ -229,6 +229,7 @@
-->
<Analyzer_MicrosoftCodeAnalysisCSharpVersion>3.3.1</Analyzer_MicrosoftCodeAnalysisCSharpVersion>
<Analyzer_MicrosoftCodeAnalysisCSharpWorkspacesVersion>3.3.1</Analyzer_MicrosoftCodeAnalysisCSharpWorkspacesVersion>
<RDG_MicrosoftCodeAnalysisVersion>4.5.0</RDG_MicrosoftCodeAnalysisVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<RDG_MicrosoftCodeAnalysisVersion>4.5.0</RDG_MicrosoftCodeAnalysisVersion>
<MicrosoftCodeAnalysisVersion_LatestVS>4.5.0</MicrosoftCodeAnalysisVersion_LatestVS>

Can we follow the same naming pattern as https://github.com/dotnet/runtime/blob/main/eng/Versions.props#LL71C6-L71C43?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add a comment above this line explaining. It would be better here than in the Generator.csproj

@captainsafia
Copy link
Member Author

Looks like you need to follow https://github.com/dotnet/source-build/blob/main/Documentation/eliminating-pre-builts.md#allowed-exceptions

dotnet/runtime's Regex source generator is already using this version (4.5.0), so it should be OK to use it in source-build.

FYI - @mthalman

I went ahead and updated the existing exemptions we had for M.CA dependencies to remove the version pin. I noticed that the runtime repo is extremely liberal about what is exempt so I assume not being picky about version numbers is fine.

@eerhardt
Copy link
Member

I noticed that the runtime repo is extremely liberal about what is exempt so I assume not being picky about version numbers is fine.

I don't think dotnet/runtime has the "pre-built" checks working yet.

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Fine by me

@wtgodbe wtgodbe requested a review from mmitche May 10, 2023 22:24
@wtgodbe
Copy link
Member

wtgodbe commented May 10, 2023

Requested review from Matt for the prebuilt baseline update

@lewing lewing requested a review from MichaelSimons May 10, 2023 23:31
@lewing lewing enabled auto-merge (squash) May 11, 2023 00:11
@lewing lewing merged commit fb88fec into main May 11, 2023
@lewing lewing deleted the safia/fix-mca-versioning branch May 11, 2023 00:32
@ghost ghost added this to the 8.0-preview5 milestone May 11, 2023
mthalman added a commit to mthalman/aspnetcore that referenced this pull request May 12, 2023
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 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 area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants