Closed Barsonax closed 4 years ago
Still getting one more warning:
Successfully created package 'C:\git\duality\Build\NuGetPackageSpecs\AdamsLair.Duality.Samples.BasicShaders.4.0.0-alpha.nupkg'.
WARNING: 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:
- Add lib or ref assemblies for the netstandard2.0 target framework
Seems this package does not contain any dll's so it does not make sense to have a target framework. Should probably be fixed by removing this from BasicShaders.Sample.nuspec
:
<dependencies>
<group targetFramework="netstandard2.0">
<dependency id="AdamsLair.Duality" version="4.0.0-alpha" />
<dependency id="AdamsLair.Duality.Editor" version="4.0.0-alpha" />
</group>
</dependencies>
I believe these dependencies were added because in the past the duality integrated package manager filtered on these dependencies @ilexp. Not really needed now anymore and even if we need it its a bit weird to use these dependencies as a filter in this case.
I believe these dependencies were added because in the past the duality integrated package manager filtered on these dependencies @ilexp. Not really needed now anymore and even if we need it its a bit weird to use these dependencies as a filter in this case.
Those are still generally needed to tie each version of a dependency to a corresponding runtime / engine version. Even if there is no source code / plugin dll included, the data files can still rely on specific versions of component implementations, serialization format details, or in this case shader format details.
That said, the editor dependency is probably not needed anymore. Not sure why it was there in the first place, but I suspect it had something to do with the package manager implementation like you mentioned.
As for the PR in its current state, looks innocent enough, but not sure whether I should actually review, given that I'm trying to reduce my involvement where possible and throw the ball to you guys more often. @SirePi Can you do this one?
Those are still generally needed to tie each version of a dependency to a corresponding runtime / engine version. Even if there is no source code / plugin dll included, the data files can still rely on specific versions of component implementations, serialization format details, or in this case shader format details.
That said, the editor dependency is probably not needed anymore. Not sure why it was there in the first place, but I suspect it had something to do with the package manager implementation like you mentioned.
Went through the packages again and noticed that in most of them AdamsLair.Duality.Editor
was listed as a dependency in the nuspec but not in the csproj itself. Changed it so that the nuspecs and csprojs are in sync again except for the Tilemaps.Sample.nuspec
as I don't see how I can easily solve this one. The problem there is that it kind of depends on AdamsLair.Duality.Editor.Plugins.Tilemaps
but the project itself is a coreplugin and not a editorplugin. Maybe we have to split it just like the DynamicLighting.Sample
or make it possible that you can have a coreplugin and a editorplugin in the same dll (for samples this would be nice I guess).
That is out of the scope of this PR though.
The DynamicLighting sample is different, because it actually implements the demonstrated dynamic lighting system, including core classes and editor integration.
The Tilemaps sample just uses the standalone Tilemap plugins for core and editor. It's code is illustrative only and as you said, the package dependency is there, because the sample can't demonstrate what it's supposed to do without the editor plugin - but of course its contained source code only requires (and should depend only on) the core plugin.
I don't think this is really an issue though - for the samples, there isn't really a need to completely sync up csproj dependencies with nuspec dependencies, and in some cases, like this one, wouldn't make sense either, since that would mean unifying two different things. For the samples, the nuspec represents the package alone, and the csproj represents just one building block that happens to be in there in compiled form. The package itself just isn't related as directly to the csproj as it would be for the usual .Net library or Duality plugin.
The DynamicLighting sample is different, because it actually implements the demonstrated dynamic lighting system, including core classes and editor integration.
It does bloat the amount of Samples on nuget though. Now a user has to choose between the core and editor variants (the user might not even be aware of this difference) and I think you always want the editor one if its available.
The Tilemaps sample just uses the standalone Tilemap plugins for core and editor. It's code is illustrative only and as you said, the package dependency is there, because the sample can't demonstrate what it's supposed to do without the editor plugin - but of course its contained source code only requires (and should depend only on) the core plugin.
I don't think this is really an issue though - for the samples, there isn't really a need to completely sync up csproj dependencies with nuspec dependencies, and in some cases, like this one, wouldn't make sense either, since that would mean unifying two different things. For the samples, the nuspec represents the package alone, and the csproj represents just one building block that happens to be in there in compiled form. The package itself just isn't related as directly to the csproj as it would be for the usual .Net library or Duality plugin.
It does make it harder to eventually transition to dotnet tooling though where the nuspecs are generated from the csproj but maybe the samples just need to be handled separately.
But yeah not big issue, its more a maybe this could be cleaner kinda thing.
It does bloat the amount of Samples on nuget though. Now a user has to choose between the core and editor variants (the user might not even be aware of this difference) and I think you always want the editor one if its available.
Yeah, that's a good point. I think the DynamicLighting sample might be well off as just "the sample", without the core or editor distinction, set up in a way to contain both core and editor plugins as well as content. I think this distinction is actually a relic from the time when DynamicLighting wasn't a sample package, but a plugin package. Samples probably shouldn't be split in two packages.
Anyway - back to topic!
Anyway - back to topic!
Yep, @SirePi if you can give this a review we can merge this
This should fix any warnings you get when running nuget pack and bring our nuget packages up to the updated spec.
Changes:
licenseUrl
withlicense
, it now uses a license expression instead of a link.PackageIconUrl
withPackageIcon
, the icon is now included in the package itselfreferences
elements as these are not needed (and actually caused warnings).