Migrated nuspec to csproj and improved packaging #54

alefranz · 2019-04-25T19:55:55Z

This fixes #36, fixes #37, fixes #38, fixes #40

Migrated from nuspec to csproj
Fixed targets location
Fixed package dependencies
Fixed package to be recognised as netstandard2.0
Moved debug symbols to symbols package
Added Sourcelink
Fixed package dependencies

Before / After

Symbols package

Package size is actually bigger now

For the Azure Pipelines I'm flying blind, but hopefully should work correctly and not break the signature.
I don't think the symbol package needs to be signed.

There's a descriptive title that will make sense to other developers some time from now.
There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
You have included any necessary tests in the same PR.

- Migrate from nuspec to csproj - Fixed targets location - Fixed package dependencies - Fixed package to be recognised as netstandard2.0 - Moved debug symbols to symbols package - Added Sourcelink - Fixed package dependencies

.gitignore

azure-pipelines.yml

examples/Microsoft.Spark.FSharp.Examples/Microsoft.Spark.FSharp.Examples.fsproj

eerhardt · 2019-04-25T22:26:10Z

- task: NuGetToolInstaller@0

I believe you can remove this NuGetToolInstaller line now that we aren't using nuget.exe

Refers to: azure-pipelines.yml:182 in 3bd65d5. [](commit_id = 3bd65d5, deletion_comment = False)

eerhardt · 2019-04-25T22:32:26Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

+
+    <PublishRepositoryUrl>true</PublishRepositoryUrl>
+    <IncludeSymbols>true</IncludeSymbols>
+    <SymbolPackageFormat>snupkg</SymbolPackageFormat>


Now we are creating .snupkg packages, but we aren't doing anything with them in the build. Can we publish them somewhere? Like in a "BuildArtifact"?

Ideally we would publish them to the Microsoft symbol server, but that is probably outside of the scope of this PR. We can do that in the future.

In reply to: 278755885 [](ancestors = 278755885)

I added a step to publish them as artifacts. Does it help?

eerhardt · 2019-04-25T22:37:38Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

+      <Pack>true</Pack>
+      <PackagePath>jars\%(Filename)%(Extension)</PackagePath>
+    </Content>
+    <Content Include="..\build\**" Link="build\%(Filename)%(Extension)">


What do you feel about moving this file into a src/csharp/Microsoft.Spark/build/netstandard2.0 folder?

I have 2 concerns:

If we add other nupkgs that need build scripts, they can't go into this root build directory, or else they would start being packed here.

The .csproj shouldn't be deciding where this file goes. Instead the file system should. If you put a file in build/foo/bar/myFile.targets, it should be in the same folder in the package.

So instead of the above, you could have:

<Content Include="build\**\*" Pack="true" PackagePath="build" />

And it would pick up any file/folder structure under the build folder.

I was trying to not move things around :) but agree it is better with this approach. Done!

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

eerhardt

This is great @alefranz! Thanks for helping out here.

safern · 2019-04-25T23:35:49Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

+      <Pack>true</Pack>
+      <PackagePath>jars\%(Filename)%(Extension)</PackagePath>
+    </Content>
+    <Content Include="..\build\**" Link="build\%(Filename)%(Extension)">


I think we should also include the License and Third-Parties files that we define on the root of the repo. This is a convention we've followed in other repos. https://github.com/dotnet/iot/blob/master/Directory.Build.props#L21

I've added that as well as moved the Licence expression and some other generic bits to the props file.
Please note this means the third party notices file will show up in the Solution Explorer for each project

How about Visible="false"?

<Content Include="$(MSBuildThisFileDirectory)..\..\THIRD-PARTY-NOTICES.TXT" Pack="true" PackagePath="\" Visible="false"/>

Yes, let's fix this on a follow up PR 😄

safern

Thanks @alefranz 😄

alefranz · 2019-04-28T17:45:03Z

- task: NuGetToolInstaller@0
I believe you can remove this NuGetToolInstaller line now that we aren't using nuget.exe

Refers to: azure-pipelines.yml:182 in 3bd65d5. [](commit_id = 3bd65d5, deletion_comment = False)

@eerhardt - I thought this was needed to install the MicroBuildSigningPlugin, can you confirm it is not needed? I can try to import the pipeline so I can test this job (as it doesn't run for PRs)

eerhardt · 2019-04-29T15:06:13Z

I thought this was needed to install the MicroBuildSigningPlugin

I don't believe this is needed for the MicroBuildSigningPlugin. We are using an older version of that plugin on ML.NET https://github.com/dotnet/machinelearning/blob/master/build/vsts-ci.yml#L134, and it doesn't require the NuGetToolInstaller. Maybe @safern knows for sure, but I believe we can remove the NuGetToolInstaller.

eerhardt · 2019-04-29T15:14:13Z

<None Include="$(MSBuildThisFileDirectory)../jars/microsoft-spark*.jar">

Is this still the right path now that this file has moved?

Refers to: src/csharp/Microsoft.Spark/build/netstandard2.0/Microsoft.Spark.targets:3 in 008abc5. [](commit_id = 008abc5, deletion_comment = False)

eerhardt · 2019-04-29T15:17:26Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

+
+  <ItemGroup>
+    <Content Include="..\..\scala\microsoft-spark-*\target\microsoft-spark-*.jar" Link="jars\%(Filename)%(Extension)"
+      Pack="true" PackagePath="jars\%(Filename)%(Extension)" />


(nit) A more readable multi-line format here would be:

<Content Include="..\..\scala\microsoft-spark-*\target\microsoft-spark-*.jar" Link="jars\%(Filename)%(Extension)" Pack="true" PackagePath="jars\%(Filename)%(Extension)" />

It's easier to see each attribute. You don't have to parse the string, or miss something way off to the right.

alefranz · 2019-04-29T16:00:28Z

@eerhardt

I removed the NugetInstall step.

I'm still not able to test on my Azure DevOps account as I don't know how to get the Signing plugin.
Could not queue the build because there were validation errors or warnings. Job Package: Step references task 'MicroBuildSigningPlugin' at version '2' which does not exist.

If you could please try to manually run a build of this PR so the Build.Reason will not be PullRequest and it will run all the tasks, so we can verify that it works correctly.

<None Include="$(MSBuildThisFileDirectory)../jars/microsoft-spark*.jar">
Is this still the right path now that this file has moved?

I haven't moved the jars, as they are installed by maven. Not sure if needed and in scope.

eerhardt · 2019-04-29T16:16:47Z

I haven't moved the jars, as they are installed by maven. Not sure if needed and in scope.

You didn't move the jars, but you did move the .targets file that is referencing the jars using $(MSBuildThisFileDirectory)../jars/microsoft-spark*.jar. $(MSBuildThisFileDirectory) is the directory that the current MSBuild file (i.e. the Microsoft.Spark.targets file) is contained in. Since you moved the .targets file down a directory, you need two ../ here to move up 2 directories.

eerhardt · 2019-04-29T16:31:27Z

I've kicked off a test of the official build using your changes @alefranz. I'll let you know how it goes.

alefranz · 2019-04-29T16:36:57Z

I haven't moved the jars, as they are installed by maven. Not sure if needed and in scope.

You didn't move the jars, but you did move the .targets file that is referencing the jars using $(MSBuildThisFileDirectory)../jars/microsoft-spark*.jar. $(MSBuildThisFileDirectory) is the directory that the current MSBuild file (i.e. the Microsoft.Spark.targets file) is contained in. Since you moved the .targets file down a directory, you need two ../ here to move up 2 directories.

Ah sorry, I didn't realize you were referring to the targets file. Fixed it.

eerhardt

I ran a test build on commit 623de33 (the one before the jar path fix), and the build passed. So I verified we can remove NuGetToolInstaller task.

Thanks for the fix @alefranz!

eerhardt · 2019-04-29T17:16:55Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

+
+  <ItemGroup>
+    <Content Include="..\..\scala\microsoft-spark-*\target\microsoft-spark-*.jar"
+      Link="jars\%(Filename)%(Extension)"


(nit) I wasn't clear about the formatting, but we typically align the attributes underneath the attribute on the first line. This isn't 100% necessary to fix, but if you are changing something else, consider this.

no worries, fixed

alefranz · 2019-04-29T17:58:06Z

Updated top of description to correctly specify the issues it fixes, also added #40

safern · 2019-04-29T18:37:26Z

eng/Sign.proj

@@ -36,7 +36,7 @@
    </ItemGroup>

    <ItemGroup Condition="'$(SignNugetPackages)' == 'true'">
-      <FilesToSign Include="$(OutDir)*.nupkg" Exclude="$(OutDir)*.symbols.nupkg">
+      <FilesToSign Include="$(OutDir)*.nupkg">


I think we should also sign snupkg, right? Since the plan is for it to go into nuget? @eerhardt what do you think?

Opps we merged right at the same moment I commented on this, but @imback82 I think we want to solve this.

I guess it depends on what we plan on doing with them... if we just use it to publish the symbols to the Microsoft Symbol Server during the build, they probably don't need to be signed.
But if we do plan on uploading them to nuget.org, then maybe they need to be signed?

@dagood @mikem8361 - for thoughts/recommendations on what should be done with .snupkg files.

@tmat knows the most about snupkgs, I think.

snupkgs should be published to NuGet.org and the PDBs to Microsoft Symbol servers.

Follow up to dotnet#54

Follow up to #54

tmat · 2019-04-29T19:40:14Z

Have you considered using Arcade SDK? It's kind of sad to see folks spending a time on their infrastructure reinventing the wheel for the hundredth time when the same work has already been done and it's easy to use.

alefranz added 2 commits April 25, 2019 20:47

Migrated nuspec to csproj and improved packaging

f3ac4ac

- Migrate from nuspec to csproj - Fixed targets location - Fixed package dependencies - Fixed package to be recognised as netstandard2.0 - Moved debug symbols to symbols package - Added Sourcelink - Fixed package dependencies

Merge branch 'master' into package

3bd65d5

alefranz commented Apr 25, 2019

View reviewed changes

.gitignore Show resolved Hide resolved

alefranz commented Apr 25, 2019

View reviewed changes

azure-pipelines.yml Show resolved Hide resolved

imback82 requested review from safern, eerhardt and imback82 April 25, 2019 20:00

imback82 added the enhancement New feature or request label Apr 25, 2019

imback82 reviewed Apr 25, 2019

View reviewed changes

azure-pipelines.yml Show resolved Hide resolved

eerhardt reviewed Apr 25, 2019

View reviewed changes

examples/Microsoft.Spark.FSharp.Examples/Microsoft.Spark.FSharp.Examples.fsproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 25, 2019

View reviewed changes

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj Outdated Show resolved Hide resolved

eerhardt reviewed Apr 25, 2019

View reviewed changes

safern reviewed Apr 25, 2019

View reviewed changes

alefranz added 3 commits April 28, 2019 18:45

Addressed feedback

5540015

publish symbol packages

69b7cef

Merge branch 'master' into package

008abc5

eerhardt reviewed Apr 29, 2019

View reviewed changes

PR feedback

c5b3ae5

Merge branch 'master' into package

623de33

Fixed jars path in target

117c2c2

eerhardt previously approved these changes Apr 29, 2019

View reviewed changes

Fixed csproj alignment

f599adf

alefranz dismissed eerhardt’s stale review via f599adf April 29, 2019 17:54

eerhardt approved these changes Apr 29, 2019

View reviewed changes

imback82 merged commit 649c192 into dotnet:master Apr 29, 2019

safern reviewed Apr 29, 2019

View reviewed changes

eerhardt added a commit to eerhardt/spark that referenced this pull request Apr 29, 2019

Hide the 3rd party notice in the solution explorer

3a143a2

Follow up to dotnet#54

eerhardt mentioned this pull request Apr 29, 2019

Hide the 3rd party notice in the solution explorer #67

Merged

imback82 pushed a commit that referenced this pull request Apr 29, 2019

Hide the 3rd party notice in the solution explorer (#67)

3776346

Follow up to #54

Migrated nuspec to csproj and improved packaging #54

Migrated nuspec to csproj and improved packaging #54

Uh oh!

Conversation

alefranz commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

alefranz commented Apr 28, 2019

Uh oh!

eerhardt commented Apr 29, 2019

Uh oh!

eerhardt commented Apr 29, 2019

Uh oh!

eerhardt Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alefranz commented Apr 29, 2019

Uh oh!

eerhardt commented Apr 29, 2019

Uh oh!

eerhardt commented Apr 29, 2019

Uh oh!

alefranz commented Apr 29, 2019

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alefranz commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alefranz commented Apr 25, 2019 •

edited

Loading

eerhardt Apr 29, 2019 •

edited

Loading

tmat commented Apr 29, 2019 •

edited

Loading