dotnet / corefxlab

This repo is for experimentation and exploring new ideas that may or may not make it into the main corefx repo.
MIT License
1.46k stars 345 forks source link

Add Microsoft.Data.Analysis.nuget project #2933

Closed dcostea closed 4 years ago

dcostea commented 4 years ago

Microsoft.Data.Analysis.nuget project was added for including Microsoft.Data.Analysis.Interactive project.

cc @eerhardt

pgovind commented 4 years ago

I'll do this later today @dcostea. Edit: nvm, it looks like the PR was already rebased.

dcostea commented 4 years ago

Edit: nvm, it looks like the PR was already rebased. @pgovind, Is it anything left to do, from my side?

eerhardt commented 4 years ago

I should have caught this last time, but we put the original Interactive project in the wrong place. It is in the root, and not under src. Can you delete that project (or logically move it to this location)?


Refers to: src/Microsoft.Data.Analysis.Interactive/Microsoft.Data.Analysis.Interactive.csproj:1 in fbe6dba. [](commit_id = fbe6dba85b8d058357fa4df727b263f3992ae8b2, deletion_comment = False)

eerhardt commented 4 years ago

It looks like the builds are actually failing, but for some reason Azure DevOps is reporting they still pass:

https://dev.azure.com/dnceng/public/_build/results?buildId=721436&view=logs&j=98d43ca7-10c7-5044-d8dc-07206263d511&t=acc458f0-2e0e-5219-28cc-1f016a65ca57&l=250

/home/vsts/work/1/s/.dotnet/sdk/5.0.100-preview.6.20310.4/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [/home/vsts/work/1/s/src/Microsoft.Data.Analysis.nuget/Microsoft.Data.Analysis.nuget.csproj]
/home/vsts/work/1/s/.dotnet/sdk/5.0.100-preview.6.20310.4/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): error NU5128: - Add lib or ref assemblies for the netcoreapp3.1 target framework [/home/vsts/work/1/s/src/Microsoft.Data.Analysis.nuget/Microsoft.Data.Analysis.nuget.csproj]
##[error].dotnet/sdk/5.0.100-preview.6.20310.4/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): error NU5128: (NETCORE_ENGINEERING_TELEMETRY=Build) Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below:
- Add lib or ref assemblies for the netcoreapp3.1 target framework

@pgovind - do you know why the CI infrastructure isn't failing here?

@dcostea - you can repro this locally by executing .\build.cmd -pack from the root of the repo.

dcostea commented 4 years ago

you can repro this locally by executing .\build.cmd -pack from the root of the repo

I saw that too and I didn't know what to think.

See below the result on my machine:

Build succeeded.

C:\repos\corefxlab\.dotnet\sdk\5.0.100-preview.6.20310.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [C:\repos\corefxlab\src\Microsoft.Data.Analysis.nuget\Microsoft.Data.Analysis.nuget.csproj]
C:\repos\corefxlab\.dotnet\sdk\5.0.100-preview.6.20310.4\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error NU5128: - Add lib or ref assemblies for the netcoreapp3.1 target framework [C:\repos\corefxlab\src\Microsoft.Data.Analysis.nuget\Microsoft.Data.Analysis.nuget.csproj]
    0 Warning(s)
    1 Error(s)
dcostea commented 4 years ago

@eerhardt , I have installed .NET 5 preview 6 and ./build.cmd looks good, the error does not reproduce anymore. I have added some unit tests but it seems the build machine doesn't have .NET Core 3.1. I cannot downgrade MDA.Interactive to .NET Core 3.0 to make it build successfully, because it needs .NET Core 3.1. Should I remove the freshly added unit test project?

eerhardt commented 4 years ago

I have added some unit tests but it seems the build machine doesn't have .NET Core 3.1.

You can add the 3.1 runtime here:

https://github.com/dotnet/corefxlab/blob/fb1fe923b19ea052341cff707094b49214d458dd/global.json#L5-L7

dcostea commented 4 years ago

You can add the 3.1 runtime here:

https://github.com/dotnet/corefxlab/blob/fb1fe923b19ea052341cff707094b49214d458dd/global.json#L5-L7

@eerhardt , I have the unit tests passing now, but the issue related to .NET 5 is still there. Maybe @pgovind can help with that?

pgovind commented 4 years ago

I'll take a look at the failing CI jobs and packing errors tomorrow :) Thanks for the patience on this!

pgovind commented 4 years ago

Alright, I took a look at the build failures and @joperezr helped me figure out what was going on. Basically the wrapper MDA.nuget project wouldn't actually build the MDA and MDAI projects. Also, @joperezr pointed out that making a wrapper project to include 2 other projects isn't the preferred approach here. Instead, I decided to include MDAI within the MDA nuget package for netcoreapp3.1+ scenarios (but MDAI still lives as it's own project. During -pack we just include everything from MDAI). I pushed a commit to the current branch @dcostea (I hope you don't mind!). I'm fairly certain that the notebook will pick up the formatter automatically if it's included in the MDA nuget package. I'll test it locally once we have a build

joperezr commented 4 years ago

@dcostea I took the liberty to push directly to your PR too in order to address the comments by @eerhardt I hope that was ok. After my last change, you can now call dotnet pack to both the MDA.csproj or MDAI.csproj and they will both generate the same package, which layout is as follows:

Icon.png
LICENSE
Microsoft.Data.Analysis.nuspec
THIRD-PARTY-NOTICES.TXT
[Content_Types].xml
interactive-extensions\dotnet\Microsoft.Data.Analysis.Interactive.dll
lib\netstandard2.0\Microsoft.Data.Analysis.dll
lib\netstandard2.0\Microsoft.Data.Analysis.xml
pgovind commented 4 years ago

I saw a demo of this locally. Super cool! I'm going to go ahead and merge it. Thanks for all the work @dcostea @eerhardt @jonsequitur and @joperezr

dcostea commented 4 years ago

Thank you, everyone, @eerhardt @pgovind @jonsequitur @joperezr @colombod for your support. I hope this is my first contribution from a long series.