-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Refactor official build to use arcade templates #6412
Conversation
This lets us remove an unnecessary copy step
We need to include top-level variable groups but can only do so if we use -name/value syntax. Hide this in a template so that it's less ugly.
@@ -1,7 +1,6 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Import Project="$(RepoRoot)eng/pkg/Pack.props" /> | |||
<PropertyGroup> | |||
<Authors>Intel</Authors> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was wrong for us to claim Intel is the Author of the package. The gallery doesn't even present it to our users.
https://www.nuget.org/packages/Microsoft.ML.Mkl.Redist/2.0.0-preview.22313.1
We are the ones who build this package and binary, it's a custom linked copy of MKL. The package itself has the Intel license.
<Target Name="Test" /> | ||
|
||
<ItemGroup> | ||
<PackageDownload Include="MlNetMklDeps" Version="[$(MlNetMklDepsVersion)]" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, PackageDownload requires explicit version references. https://learn.microsoft.com/en-us/nuget/consume-packages/packagedownload-functionality
@LittleLittleCloud Any idea about this failure?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my best knowledge, LGTM.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6412 +/- ##
==========================================
- Coverage 68.54% 68.52% -0.02%
==========================================
Files 1169 1170 +1
Lines 246867 246960 +93
Branches 25788 25790 +2
==========================================
+ Hits 169206 169222 +16
- Misses 70938 71022 +84
+ Partials 6723 6716 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactors the official build to use the arcade templates which automate a lot of validation tasks. It also removes some redundant signing we were doing before, opting for a single signing phase after all packages have been built.
Here's a successful build from this change https://dev.azure.com/dnceng/internal/_build/results?buildId=2030382&view=results