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

minimize nuget deps for FSharp.Core #1358

Merged
merged 1 commit into from
Aug 1, 2016
Merged

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Jul 18, 2016

This is an attempt to minimize the dependencies in the FSharp.Core nuget package so it doesn't induce a dependency on the whole .NETStandard library

See #1357

@forki
Copy link
Contributor

forki commented Jul 18, 2016

👍 is this already running in a (integration) test? so that we can see if this would work?

@dsyme
Copy link
Contributor Author

dsyme commented Jul 18, 2016

@forki I'm not sure

@forki
Copy link
Contributor

forki commented Jul 18, 2016

I guess this means no ;-)

easiest way to check would be to remove all deps and see if we get a red light.

@cartermp
Copy link
Contributor

Disclaimer: aside from project.json I actually don't really know how NuGet works

Is it necessary to have a nuspec here when the project.json file displays package info? All of the metadata is supported by that file. Example:https://github.com/louthy/language-ext/blob/master/LanguageExt.Core/project.json

@baronfel
Copy link
Member

It may be worth trying with nuget pack Path_to_proj_file as described here. In addition, nuget will pack other projects that you have a project reference to as dependencies as long as those project have a specially-formatted nuspec template if you follow the directions here.

@forki
Copy link
Contributor

forki commented Jul 18, 2016

@cartermp I think this one is for nuget pack to create a new package. I assume latest version could infer this from project.json, but I think this is much older than that (and I'm not completely sure if it already infers the stripped down dependencies list). So since the tooling is about to change again we probably just keep it as nuspec and hope to make it work.

@cartermp
Copy link
Contributor

@forki Yeah, if you have dotnet pack -c release in the CI then it builds a NuGet package using nuget pack under the covers. But I suppose it's good to play it safe until tooling stabilizes.

@KevinRansom
Copy link
Member

I don't mean to be facetious but isn't the dependency list bigger with this change?

@forki
Copy link
Contributor

forki commented Jul 18, 2016

The transitive dependencies should be much smaller if we don't reference
the whole framework meta packages.

On Jul 18, 2016 9:03 PM, "Kevin Ransom (msft)" notifications@github.com
wrote:

I don't mean to be facetious but isn't the dependency list bigger with
this change?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1358 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNHek8lqXRyeLqtR52Cs91II1TkkQks5qW84egaJpZM4JOyt3
.

@cartermp
Copy link
Contributor

It's smaller in this case. NuGet doesn't support publish-time trimming, so the NuGet package will come packaged up with all of the packages (I think 13 or 14?) that it doesn't actually use.

@ctaggart
Copy link
Contributor

Iit looks like FSharp.Core 4.0.15 was just published to the NuGet Gallery:
https://www.nuget.org/packages/FSharp.Core/4.0.1.5

It lists these as the NuGet dependencies for .NETStandard 1.6:

Microsoft.NETCore.Platforms (>= 1.0.1)
NETStandard.Library (>= 1.6.0)
System.Linq.Expressions (>= 4.1.0)
System.Linq.Queryable (>= 4.0.1)
System.Reflection.Emit (>= 4.0.1)
System.Reflection.TypeExtensions (>= 4.1.0)
System.Runtime.Loader (>= 4.0.0)
System.Net.Requests (>= 4.0.11)
System.Threading.Tasks.Parallel (>= 4.0.1)
System.Threading.Thread (>= 4.0.0)
System.Threading.ThreadPool (>= 4.0.10)

My understanding is that this PR reduces the total number of transitive dependencies, but makes the displayed list longer since it doesn't put a dependency on the NETStandard.Library aggregate. Is that correct? That makes sense to me.

@cartermp
Copy link
Contributor

@ctaggart Yes, that's correct.

@forki
Copy link
Contributor

forki commented Jul 20, 2016

see fsharp/fsharp#590

@dsyme
Copy link
Contributor Author

dsyme commented Jul 20, 2016

@ctaggart I presume the large set of transitive dependencies will cause a lot of problems for users of the FSharp.Core package? Especially for Paket users if the framework restriction setting is not used in the dependencies file ? And possibly also for Nuget users?

If so, do you think we should unlist that nuget package until this PR is merged and a new FSharp.Core for .NET Standard is available?

Thanks
don

@forki
Copy link
Contributor

forki commented Jul 20, 2016

I think a good compromise would be to unlist and release alpha versions instead.

@isaacabraham
Copy link
Contributor

@dsyme I've already gotten into the habit of putting net452 into my paket.dependencies file - there are a few packages that already support netcore and it takes ages to download them all - not to mention that the lock file becomes huge and impossible to reason about.

@dsyme
Copy link
Contributor Author

dsyme commented Jul 20, 2016

I checked the Nuget bahaviour from the Visual Studio 2015 IDE and it doesn't seem to add any extra time to using the nuget package.

@forki I do wonder if some framework setting e.g. framework: auto-detect should be in all paket examples in the documentation.

@forki
Copy link
Contributor

forki commented Jul 20, 2016

auto-detect is currently more a experimental feature and adds some serious time penalty since we need to compute the framework from what is specified in MSBuild files

@KevinRansom
Copy link
Member

@dsyme
Thank you for taking care of this.

kevin

@KevinRansom KevinRansom merged commit 4b8298c into dotnet:master Aug 1, 2016
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.

8 participants