-
-
Notifications
You must be signed in to change notification settings - Fork 484
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
Switch from .nuspec files to project properties #958
Conversation
@EdwardCooke, I have at least one upcoming change to propose that leverages If you have any questions, please let me know. Thanks! |
I’d rather not bring in a dependency on another package. Things get messy for the consumer when they already reference a different version. |
Ok no problem. I think this change is still goodness, even without new dependencies. Otherwise I was confused for a while because there's diverging values between the project and the nuspec. Additionally, today the YAMLDotNet project is taking references that aren't declared. Luckily they also aren't used, but without this change you could get into a state where you ship a broken package. Let me know if you disagree or have questions. Thanks! |
3fc362a
to
d5ac4f4
Compare
There's also #931 , but my proposals seem go unnoticed. |
They’re not going unnoticed. I promise. I’m hoping to start knocking some prs out in the next few weeks. |
@lahma I just merged in your PR, so this PR has become obsolete so I'm going to close this one. |
@EdwardCooke, I believe this PR is still relevant. If you take a look at your tools build: YamlDotNet/tools/build/BuildDefinition.cs Line 146 in 2e317a5
You're still packing via a nuspec. If you open a built package in a tool like NuGet Package Explorer you'll see that none of the package properties in the .csproj are being applied. I believe this PR still solves a few problems, namely:
Would you mind taking another look? If I'm misunderstanding something or you have additional questions, please let me know. Thanks! |
I'll double check this then, I may have missed something. |
I just compared the package output from both the nuget pack and the dotnet pack version, theres a couple of differences in the output between the 2 (nuget vs dotnet). Don't be wrong, I want to use New with <?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
<metadata>
<id>YamlDotNet</id>
<version>16.1.0-ec-makeconcurrent-</version>
<authors>YamlDotNet</authors>
<license type="expression">MIT</license>
<licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
<projectUrl>https://github.com/aaubry/YamlDotNet</projectUrl>
<description>YamlDotNet is a .NET library for YAML. YamlDotNet provides low level parsing and emitting of YAML as well as a high level object model similar to XmlDocument. A serialization library is also included that allows to read and write objects from and to YAML streams.</description>
<copyright>Copyright (c) Antoine Aubry and contributors</copyright>
<repository type="git" url="https://github.com/aaubry/YamlDotNet" commit="5737a3c56b871b49b0856a81f00157f760872d55" />
<dependencies>
<group targetFramework=".NETFramework4.7" />
<group targetFramework="net6.0">
<dependency id="System.ComponentModel.TypeConverter" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Diagnostics.Debug" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Runtime.Serialization.Formatters" version="4.3.0" exclude="Build,Analyzers" />
</group>
<group targetFramework="net8.0">
<dependency id="System.ComponentModel.TypeConverter" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Diagnostics.Debug" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Runtime.Serialization.Formatters" version="4.3.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.0">
<dependency id="System.ComponentModel.TypeConverter" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Diagnostics.Debug" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Runtime.Serialization.Formatters" version="4.3.0" exclude="Build,Analyzers" />
</group>
<group targetFramework=".NETStandard2.1">
<dependency id="System.ComponentModel.TypeConverter" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Diagnostics.Debug" version="4.3.0" exclude="Build,Analyzers" />
<dependency id="System.Runtime.Serialization.Formatters" version="4.3.0" exclude="Build,Analyzers" />
</group>
</dependencies>
<frameworkAssemblies>
<frameworkAssembly assemblyName="System.Core" targetFramework=".NETFramework4.7" />
<frameworkAssembly assemblyName="System" targetFramework=".NETFramework4.7" />
</frameworkAssemblies>
</metadata>
</package> Original with <?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/01/nuspec.xsd">
<metadata minClientVersion="2.8">
<id>YamlDotNet</id>
<version>16.1.0-ec-makeconcurrent-</version>
<authors>Antoine Aubry</authors>
<requireLicenseAcceptance>false</requireLicenseAcceptance>
<license type="expression">MIT</license>
<licenseUrl>https://licenses.nuget.org/MIT</licenseUrl>
<icon>images/yamldotnet.png</icon>
<readme>README.md</readme>
<projectUrl>https://github.com/aaubry/YamlDotNet/wiki</projectUrl>
<iconUrl>http://aaubry.net/images/yamldotnet.png</iconUrl>
<description>A .NET library for YAML. YamlDotNet provides low level parsing and emitting of YAML as well as a high level object model similar to XmlDocument.</description>
<summary>This package contains the YAML parser and serializer.</summary>
<language>en-US</language>
<tags>yaml parser development library serialization</tags>
<repository type="git" url="https://github.com/aaubry/YamlDotNet.git" />
<dependencies>
<group targetFramework=".NETFramework4.7" />
<group targetFramework=".NETStandard2.0" />
<group targetFramework=".NETStandard2.1" />
<group targetFramework="net6.0" />
<group targetFramework="net8.0" />
</dependencies>
</metadata>
</package> |
Found a difference that isn't supported by using the project properties instead of the .nuspec. The summary. It's whats shown in Visual Studio when you search for a nuget package. So I think we should stick to the nuspec, however, we should switch it to using dotnet pack with the nuspec instead of using nuget pack so it's cross platform. |
Also, the references in the csproj for dotnet core can safely be removed. They aren't used. Not sure when or why they were added. |
Summary is deprecated in favor of description, see https://learn.microsoft.com/en-us/nuget/reference/nuspec#summary. I'll double-check that the description shows up in VS UI; assuming is does, is switching OK with you?
I did a I'm unfortunately tied up with other stuff at the moment, but I should be able to update this PR for you to take another look later this week, or on the weekend at the latest. If you have any questions / concerns, please let me know. Thanks for taking a look! |
I'm fine with the change. It was just something I noticed. No rush on it, I'm not planning on pushing a new version for about a week and a half. I try to keep releases no more frequent than every 2 weeks. |
d5ac4f4
to
297866a
Compare
297866a
to
126f2ea
Compare
@EdwardCooke, this should be ready for another look. I also validated I was able to consume the package in a project that builds both |
At first glance it looks good. I'll take a look at it tomorrow in depth. |
Both the main YamlDotNet and the StaticGenerator projects had NuGet package information split between project properties and metadata in the .nuspec file. More confusingly, the projects weren't linked to the nuspec (using
<NuspecFile>
), so the packages were totally different if build viadotnet pack
ornuget pack
.To avoid confusion and simplify future changes, I migrated all the properties from each package into project properties and deleted the nuspec files. I verified via a diff tool that the package contents and manifests are the same except for fixing bugs.
Known diffs:
requireLicenseAcceptance=false
is now the default sodotnet pack
omits it<iconUrl>
is removed (because it is deprecated) and<icon>
with a path inside the package is used insteadLICENSE.txt
is no longer included as we use the<license>
expression instead<language>
is automatically excluded because there are no localized resources<summary>
is dropped (because it is deprecated); the VS UI shows the package description in its placeHere's a diff of each:
YamlDotNet
YamlDotNet.Analyzers.StaticGenerator
Here the analyzer was incorrectly being put into
//analyzers/dotnet/cs/netstandard2.0
, when it should instead be either in the parent folder or should have a roslyn version folder. Thus the analyzer was moved to//analyzers/dotnet/cs