AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.4k stars 289 forks source link

Duality package structure and nuspec is wrong #743

Closed Barsonax closed 4 years ago

Barsonax commented 5 years ago

Summary

Duality now targets .NET 4.7.2 but in the lib folder there is still a net45 map: image

I believe this should be changed to net472.

Also dependencies should be in a group (replace netstandard with netframework ofcourse):

    <dependencies>
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.Reflection.Emit.Lightweight" version="4.3.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>

And not like this:

    <dependencies>
      <dependency id="AdamsLair.Duality" version="3.1.3" />
      <dependency id="AdamsLair.DockPanelSuite" version="2.8.2" />
      <dependency id="AdamsLair.TreeViewAdv" version="1.7.7" />
      <dependency id="AdamsLair.WinForms" version="1.1.17" />
      <dependency id="AdamsLair.WinForms.PopupControl" version="1.0.1" />
      <dependency id="NuGet.Core" version="2.14.0" />
    </dependencies>

Pretty sure the current package structure will confuse nuget as it thinks its still a net45 package and not a net472 package. You also get tons of warnings in visual studio: image

I think we should fix this before proceeding with the nuget upgrade #740

ilexp commented 5 years ago

Yeah, the target framework in the package specs should be fixed. It's probably not critical, but important.

The ungrouped dependency specs are fine though according to the docs. Groups can be specified, but it's optional if they're the same across frameworks anyway. If we choose to do grouping anyway, we should probably go with a single group that doesn't specify a framework to indicate that it's the default. Might as well do that while we're at it.

ilexp commented 5 years ago

Note: This only applies to .NET Framework packages (editor, editor plugins, launcher, backend), not PCL packages (core, core plugins, samples). A PCL packages framework folders would only change in the course of #573.

Barsonax commented 5 years ago

The ungrouped dependency specs are fine though according to the docs.

When you generate a nuspec with dotnet pack it will always include the groups (even if there is just 1 group) with the corresponding targetframework. Nuget figures out the rest (such as compatibility). Might as well include . netframework 4.7.2.

The version without groups still works ofcourse but might get deprecated at some point.

ilexp commented 5 years ago

ToDo

huffSamuel commented 4 years ago

I implemented a fix for this on my fork of this repository. Since this is my first contribution to this repo, do you prefer doing an external review of my branch or is it preferred that I submit a PR and review through there?

ilexp commented 4 years ago

Sorry for the long delay, I'm pretty short on time lately - a Pull Request is probably the best way to go about this 🙂 Better visibility to project contributors for review, and also a good starting point for collaborating on your feature branch as the situation permits.