Uh oh!
There was an error while loading. Please reload this page.
Create a redist package for tensorflowCreate a nuget package that red… - #720
Create a redist package for tensorflowCreate a nuget package that red…#720ericstj wants to merge 6 commits into
Conversation
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Content Include="..\common\CommonPackage.props" Pack="true" PackagePath="build\netstandard2.0\$(MSBuildProjectName).props" /> |
There was a problem hiding this comment.
Question (probably for a lawyer): Do we need to put a license/notice in this package? or is using the PackageLicenseUrl enough to meet TF requirements? #Resolved
There was a problem hiding this comment.
We have a discussion going around this and will find out all the details.
In reply to: 212419037 [](ancestors = 212419037)
There was a problem hiding this comment.
TF included a license in their zips so I went ahead and placed it in the root of this package.
In reply to: 212426813 [](ancestors = 212426813,212419037)
| @@ -0,0 +1,133 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk" DefaultTargets="Restore;Build"> | |||
There was a problem hiding this comment.
Do you really need Restore? I don't see any PackageReference #Resolved
There was a problem hiding this comment.
SDK has a hard dependency. There's an implicit package reference, but even when I disabled it the SDK still complained.
In reply to: 212420522 [](ancestors = 212420522)
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <DisableImplicitFrameworkReferences>true</DisableImplicitFrameworkReferences> | ||
| <CopyBuildOutputToOutputDirectory>false</CopyBuildOutputToOutputDirectory> |
There was a problem hiding this comment.
Could this just be a <Project> (note without an Sdk), and just import Directory.Build.props/targets itself? Then we wouldn't need to try to coerce the .NET.Sdk at all.
We could also just use the Build target, instead of overriding the CoreCompile target... See \src\Native\build.proj. #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd lose the clean support then. I guess I could write my own.
Initially I was trying to make this participate in the SLN so I wanted more of the normal targets, but I ran into issues with VS not liking the extension and .msbuildproj being broken with SDK-style projects.
I'll go ahead and get rid of the SDK.
In reply to: 212421920 [](ancestors = 212421920,212421508)
There was a problem hiding this comment.
I don't know if this helps or hurts (so don't take this as a "I think you should do this"). I'm just throwing it out there for discussion purposes.
There is a "NoTargets" Sdk that is supposed to help in these types of scenarios. I don't know how I feel about being dependent on this (I've never taken a dependency on it in a production project), but it is supposed to solve this scenario. #Resolved
There was a problem hiding this comment.
I don't think we actually need it. I took a look and it doesn't help much. It doesn't pull in common targets so I'd still need to roll my own clean. It also looks like the file extension it's using in samples is csproj. Not sure I like that. Feels cleaner to just use vanilla msbuild and let it be something that requires a full build of the repo before working.
In reply to: 212431038 [](ancestors = 212431038)
There was a problem hiding this comment.
Sounds good to me. Thanks for taking a look. #Resolved
| <!-- The archives are valid, lets extract them, ensuring an empty directory --> | ||
| <RemoveDir Directories="@(TensorFlowArchive->'%(ExtractDirectory)')" /> | ||
| <MakeDir Directories="@(TensorFlowArchive->'%(ExtractDirectory)')" /> | ||
| <ZipFileExtractToDirectory Condition="'%(FileExtension)' == '.zip'" |
There was a problem hiding this comment.
(nit) To be complete, shouldn't this be '%(TensorFlowArchive.FileExtension)'? Same for the .tar.gz check below. #Resolved
Create a nuget package that redistributes the TensorFlow C-API. This is needed because TensorFlow doesn't ship an official NuGet package. This is a straight up repack of the bits published on tensorflow.org. I made sure to apply the TensorFlow license to this package and not sign it with our authenticate certificates.
Remove the use of the SDK targets, and define our own build and clean.
| @@ -0,0 +1,114 @@ | |||
| <Project> | |||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), Directory.Build.props))\Directory.Build.props" /> | |||
There was a problem hiding this comment.
[](start = 0, length = 2)
question : do we have a need to package the gpu-libraries as well ? #Closed
There was a problem hiding this comment.
Not in this PR at least. TF only appears to publish the linux GPU build and the files have the same name as CPU build. We can consider doing something in the future, but I'd hope we have a better story than linux-only. Perhaps when we build them ourselves. Then we need to decide the user gesuture for selection: is it a property set in the project? package-reference?
In reply to: 212439593 [](ancestors = 212439593)
Tar on windows was failing when msbuild passed it a full path. Workaround by using relative paths and running where we extract to.
ericstj
commented
Aug 24, 2018
This has been combined with #704 |
Fixes#713
Create a nuget package that redistributes the TensorFlow C-API.
This is a straight up repack of the bits published on tensorflow.org. I made sure to apply the TensorFlow license to this package and not sign it with our authenticate certificates.
This is part 1 of the TF packaging. Once @abgoswam merges #704 I plan to refactor that into its own package and depend on this (as well as update the tests to use the binaries from this redist project). I can either do that as a separate PR or as part of this.