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

Enable publishing in VMR #7233

Merged
merged 4 commits into from
Mar 14, 2024
Merged

Conversation

NikolaMilosavljevic
Copy link
Member

Contributes to dotnet/source-build#4101

Enables publishing in VMR.

This repo creates packages in locations that aren't collected by common publishing infra in arcade. We need to collect them here.

@NikolaMilosavljevic NikolaMilosavljevic requested review from ViktorHofer, mmitche and a team March 7, 2024 23:07
@NikolaMilosavljevic NikolaMilosavljevic requested a review from a team as a code owner March 7, 2024 23:07
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.47%. Comparing base (0e9cb2a) to head (f8fa09b).
Report is 17 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7233    +/-   ##
========================================
  Coverage   96.46%   96.47%            
========================================
  Files        1427     1436     +9     
  Lines      341805   342787   +982     
  Branches    11259    11289    +30     
========================================
+ Hits       329725   330693   +968     
- Misses       9231     9239     +8     
- Partials     2849     2855     +6     


<ItemGroup>
<_PackagesToPublish Remove="@(_PackagesToPublish)" />
<_PackagesToPublish Include="$(ArtifactsPackagesDir)**\*.nupkg" UploadPathSegment="Roslyn-analyzers" Condition="'$(DotNetBuildRepo)' == 'true'" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm skeptical any of this is needed. In addition, UploadPathSegment won't do anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very similar to the fsharp case: dotnet/fsharp#16838 (comment). Arcade infra won't collect all of the packages as the sub-directories under artifacts/Packages are different than what arcade expects.

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't collect everything I think. This will include duplicates of packages (with different branding).

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Mar 8, 2024

Choose a reason for hiding this comment

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

Old model of harvesting artifacts (RepoManifest.xml) was collecting all of these. Here's are the packages:

artifacts/packages/Release/PreRelease/Microsoft.CodeAnalysis.Analyzers.3.11.0-beta1.final.nupkg
artifacts/packages/Release/PreRelease/Microsoft.CodeAnalysis.AnalyzerUtilities.3.11.0-beta1.final.nupkg
artifacts/packages/Release/PreRelease/Microsoft.CodeAnalysis.NetAnalyzers.9.0.0-preview.final.nupkg
artifacts/packages/Release/PreRelease/Roslyn.Diagnostics.Analyzers.3.11.0-beta1.final.nupkg
artifacts/packages/Release/Release/Microsoft.CodeAnalysis.Analyzers.3.11.0.nupkg
artifacts/packages/Release/Release/Microsoft.CodeAnalysis.AnalyzerUtilities.3.11.0.nupkg
artifacts/packages/Release/Release/Microsoft.CodeAnalysis.NetAnalyzers.9.0.0.nupkg
artifacts/packages/Release/Release/Roslyn.Diagnostics.Analyzers.3.11.0.nupkg
artifacts/packages/Release/Shipping/Microsoft.CodeAnalysis.Analyzers.3.11.0-beta1.24128.1.nupkg
artifacts/packages/Release/Shipping/Microsoft.CodeAnalysis.AnalyzerUtilities.3.11.0-beta1.24128.1.nupkg
artifacts/packages/Release/Shipping/Microsoft.CodeAnalysis.NetAnalyzers.9.0.0-preview.24128.1.nupkg
artifacts/packages/Release/Shipping/Roslyn.Diagnostics.Analyzers.3.11.0-beta1.24128.1.nupkg

No duplication of package names, but I'm unsure of contents and if we need all of them. Linux VMR build did require more than what was harvested by default arcade publishing infra.

I'm doing a local test to see if we can avoid changes in this PR altogether. Same with fsharp changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - it appears that none of the other packages are needed from this repo - I'm going to close this PR once I'm done with verification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to remove all of these packages and 2nd-stage Linux build failed with missing dependency (package downgrade):

##vso[task.logissue type=error;sourcepath=/src/git/dotnet/src/sdk/src/BuiltInTools/dotnet-watch/dotnet-watch.csproj;linenumber=0;columnnumber=0;code=NU1605;](NETCORE_ENGINEERING_TELEMETRY=Restore) Warning As Error: Detected package downgrade: Microsoft.CodeAnalysis.AnalyzerUtilities from 3.11.0 to 3.11.0-beta1.24128.1. Reference the package directly from the project to select a different version. %0A dotnet-watch -> Microsoft.CodeAnalysis.CSharp.Features 4.10.0-3.24129.7 -> Microsoft.CodeAnalysis.Features 4.10.0-3.24129.7 -> Microsoft.CodeAnalysis.AnalyzerUtilities (>= 3.11.0) %0A dotnet-watch -> Microsoft.CodeAnalysis.CSharp.Features 4.10.0-3.24129.7 -> Microsoft.CodeAnalysis.AnalyzerUtilities (>= 3.11.0-beta1.24128.1)

I am now testing a change to include the following packages, and only in source-only build:

artifacts/packages/Release/Release/Microsoft.CodeAnalysis.Analyzers.3.11.0.nupkg
artifacts/packages/Release/Release/Microsoft.CodeAnalysis.AnalyzerUtilities.3.11.0.nupkg
artifacts/packages/Release/Release/Microsoft.CodeAnalysis.NetAnalyzers.9.0.0.nupkg
artifacts/packages/Release/Release/Roslyn.Diagnostics.Analyzers.3.11.0.nupkg

Copy link
Member Author

Choose a reason for hiding this comment

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

Trimmed the publishing to include minimum required packages - f62d4c4

Without these changes, 2nd stage Linux build is failing. This can be evaluated at later time to determine if there is a way to get rid of these additional packages. Linux source-build was requiring all of these packages and more. This is an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a reference to tracking issue with 109356e

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Same as in symreader and fsharp, please change this to not require a target. When you are already at it, please also remove the PublishingVersion property and the xml tag line at the very top of the file.

@NikolaMilosavljevic
Copy link
Member Author

Same as in symreader and fsharp, please change this to not require a target. When you are already at it, please also remove the PublishingVersion property and the xml tag line at the very top of the file.

Thanks!

@NikolaMilosavljevic
Copy link
Member Author

Same as in symreader and fsharp, please change this to not require a target. When you are already at it, please also remove the PublishingVersion property and the xml tag line at the very top of the file.

Fixed with f8fa09b

@ViktorHofer
Copy link
Member

@dotnet/roslyn-analysis can you please approve and merge this PR?

@carlossanlop carlossanlop merged commit 82f8a1f into dotnet:main Mar 14, 2024
11 checks passed
<ItemGroup>
<ItemsToPushToBlobFeed Include="$(ArtifactsPackagesDir)Release\*.nupkg"
IsShipping="true"
UploadPathSegment="Roslyn-analyzers"
Copy link
Member

Choose a reason for hiding this comment

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

I don't see Arcade respecting the UploadPathSegment item metadata. Why did you have to set it?

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.

4 participants