Suggestion: modernize the project files and generate the package with dotnet pack - #134
Conversation
Replaces the hand-maintained nuspec with SDK packaging, so the dependency groups follow the real TargetFrameworks. The nuspec had drifted: it declared net6.0 and net7.0 groups that are not built, and no group for net10.0, which is shipped. Drops Microsoft.SourceLink.GitHub — SourceLink ships in the .NET SDK since 8.0.100 and the repository metadata (branch, commit) now lands in the package without it. Centralizes package versions in src/Directory.Packages.props. AngleSharp and Jint are stated as ranges because `dotnet pack` publishes them verbatim; the 2.0 / 5.0 ceilings the nuspec carried are preserved, and the ceiling now holds even when CI overrides the floor via -AngleSharpVersion. Points the test project at net10.0 and drops the netstandard2.0 pin on its ProjectReference, so tests exercise the library build they ship against rather than always the netstandard2.0 one. Also converts the solution to .slnx (which drops four orphaned project-config blocks), moves both AssemblyInfo.cs files into the csproj, adds a global.json, and upgrades NUKE to 10.1.0. The produced package is byte-comparable to the one devel produces, apart from the net10.0 group being added, the dead net6.0/net7.0 groups being removed, and SourceLink metadata being present. Assembly identity is unchanged: Version=1.0.0.0, PublicKeyToken=e83494dcdc6d31ea. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Suggestion 1 is a contradiction. On the one hand it claims that the generated NuGet package had the right version range, on the other hand it brought up "1.*" which should not be there at all if the right variable would have been introduced. As it stands both ways (directly via dotnet and from a nuspec via nuget) are, unfortunately, not equivalent. I wish the .NET team gets their stuff together and allow to specify all items of a nuspec within the csproj. |
FlorianRappl
left a comment
There was a problem hiding this comment.
Some of the changes are fine - even though I would not love to see them in isolation. Either we update all repositories or none. The last thing I want is inconsistency.
However, in general any change that does not bring a clear benefit and has drawbacks (e.g., migration to slnx - this is certainly the right step but I don't see a clear benefit to just change it) is not desired.
Reverts the .slnx conversion per review: the format change is not worth narrowing the tooling contributors can use, and the benefit was small. Keeps the part that was actually broken — four ProjectConfiguration blocks referencing project GUIDs that no longer exist in the solution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — both fair hits. Taking them in order. The
|
| Difference | Assessment |
|---|---|
owners dropped |
Deprecated by NuGet; nuget.org ignores it in favour of account ownership. Your call whether that's a loss. |
requireLicenseAcceptance dropped |
Omission and false are the same thing to NuGet. |
repository gains branch + commit |
Gain — this is SourceLink working without the package. |
exclude="Build,Analyzers" added |
Standard pack output; it spells out the default PackageReference asset behaviour rather than changing it. |
And the substantive part: the checked-in nuspec declares net6.0 and net7.0, which aren't built, and no net10.0, which is shipped. That's the drift I'd want to fix; net10.0 consumers currently resolve against the net8.0 group.
Dependency ranges come out byte-identical — [1.5.0, 2.0.0) and [4.15.3, 5.0.0). So does everything else in the package:
$ diff <(cd devel-pkg && find . -type f | sort) <(cd dotnet-pack-pkg && find . -type f | sort)
16c16
< ./package/services/metadata/core-properties/8ea1fcb73d824bff8dbd66c842f651d2.psmdcp
---
> ./package/services/metadata/core-properties/9295664c72984b799687dd3d31f835cf.psmdcp
That's the per-pack random OPC metadata name — every lib/ entry, logo.png, and README.md matches. Assembly identity is also unchanged, which was the thing I most wanted to be sure of given the strong naming:
devel AngleSharp.Js, Version=1.0.0.0, Culture=neutral, PublicKeyToken=e83494dcdc6d31ea
this PR AngleSharp.Js, Version=1.0.0.0, Culture=neutral, PublicKeyToken=e83494dcdc6d31ea
FileVersion, InformationalVersion, the InternalsVisibleTo public key and ComVisible all match too. The one deliberate difference is AssemblyCopyright, which read Copyright © AngleSharp, 2013-2019 in AssemblyInfo.cs while the nuspec said 2017-2026; they now agree on the latter.
If owners or any other nuspec-only field is something you want to keep, then the nuspec route is the right call and Suggestion 1 should just be dropped — the rest of the PR doesn't depend on it.
slnx
Reverted in ce2bb37 — you're the one who has to live with the contributor surface, and "not much benefit right now" is hard to argue with when the only concrete gain was four dead config blocks.
Those I kept, as a plain .sln edit. They were ProjectConfiguration entries for four project GUIDs that aren't declared in the solution at all, so the diff against devel is just their removal and nothing else.
Removes the global.json added earlier in this branch, per review: the absence is intentional, not an oversight, and AGENTS.md now says so rather than describing it as a gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On a bit parallel note, I dropped NUKE in my own projects already in favor of https://fallout.build/ . Pretty easy migration and fixed the pain that I had about the old GitHub Actions versions and also vulnerable dependencies. |
|
The four dead blocks we can also remove right now - it does not justify a transition (and removing those dead blocks would be appreciated anyway). The other thing is all fine to me - thanks for the showing the diff. Now the only thing to stands between merging this is consistency. I think this way would work for all (exceptions apply) AngleSharp repositories (Css, Io, Xml, Renderer) - we only would need to check the core repository (AngleSharp). My guess is that it would work there, too - but here we need to be a bit more cautious to achieve alignment. Regarding Fallout: Yes, we can migrate (sounds fine to me as NUKE is unfortunately dead) but here again I'd prefer a consistent solution (i.e., must be applied to the other repositories in the AngleSharp organization, too). The old mechanism with the GitBase repo (https://github.com/AngleSharp/AngleSharp.Infrastructure.GitBase) is anyway dead; so this one can be ignored. |
|
Maybe I'll try a run for a draft PR against main AngleSharp repo and we can check the package diffs. The ContinuousIntegrationBuild = true should for example fix the warning on NuGet package explorer: https://nuget.info/packages/AngleSharp.Js/1.0.0-beta.137 . Fallout migration is not that complex, I even developed a Claude skill for that purpose. But I guess that would be a separate PR, if wanted. |
|
Yes, let's keep it separate. I think the migrate script should be enough (as its a hard-fork I only expect namespaces + package names to change; code can remain as-is). |
Two things the core repo already does better, found while porting this change to AngleSharp/AngleSharp#1272. Drops the AssemblyVersion pin. Published AngleSharp bumps AssemblyVersion with every minor (1.5.0.0, 1.6.0.0, 1.7.0.0), so freezing it here would have made the two packages follow opposite policies. Restores <Version> as the fallback for builds that bypass the NUKE script, matching the core layout. The built assembly is unchanged either way at 1.0.0.0. Replaces the GITHUB_ACTIONS environment gate for ContinuousIntegrationBuild with .SetContinuousIntegrationBuild(IsServerBuild) on Compile and pack, which is what nuke/Build.cs in the core repo does. NUKE's host detection is not tied to one CI provider, and the core repo's published symbols prove the mechanism works: AngleSharp 1.6.0 has zero absolute paths. Verified: package metadata and dependency ranges are byte-identical to the previous state of this branch apart from the SourceLink commit hash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Did the core repository run: AngleSharp/AngleSharp#1272 (draft, with the package diff). It works there, and the delta is smaller than here — the nuspec's only drift was two dead groups ( Two things the core repo does better than what I had here, both now fixed in f6fbd93: 1. AssemblyVersion. I had pinned it to So the house policy is the opposite of my pin, and keeping it would have left the two packages following contradictory rules. Pin dropped; 2. Determinism. You had this right already in the core repo — The published symbols show it working. AngleSharp 1.6.0: AngleSharp.Js 1.0.0-beta.137, which has no such call: That This branch now uses the core repo's mechanism, verified by building with the Actions host detected: Package metadata and dependency ranges are byte-identical to the previous state of this branch apart from the SourceLink commit hash, so neither change moves the package. Dead solution blocks — already done in ce2bb37, as a plain On the remaining repositories (Css, Io, Xml, Renderer): happy to do them, but I would rather land these two first so there is an agreed shape to copy, and only one review of the argument. One thing that will recur — moving a test project to Fallout: agreed it is a separate change, and agreed it should go across the organisation at once. Not touched here. |
…uspec Mirrors AngleSharp/AngleSharp.Js#134 so the two repositories package the same way. src/AngleSharp.nuspec had drifted from what the project builds: it declared net6.0 and net7.0 dependency groups for frameworks that are not in TargetFrameworks. Generating the metadata from the csproj means the groups follow TargetFrameworks by construction, and System.Text.Encoding.CodePages lands only on the TFMs whose condition selects it. Package versions move to src/Directory.Packages.props (CPM). CHANGELOG.md becomes the single version authority: nuke/Build.cs already parsed it for the package version and now passes it as -p:Version, so <Version> in Directory.Build.props is just a fallback for builds that bypass the script. Moves the test project to net10.0, which raises it to the .NET 10 AnalysisLevel and surfaced CA2022 in two places that ignored a Stream.Read return value. Both are fixed rather than suppressed. Verified against the package devel produces: identical file listing, identical dependency ranges, and unchanged assembly identity (Version=1.7.0.0, PublicKeyToken=e83494dcdc6d31ea, same FileVersion and InformationalVersion). 3741 tests pass on net10.0, net462 and net472 in both prefetched modes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nobody asked for this — it came out of dogfooding the repo's build while working on something else. Offering it as a suggestion rather than a fix: each piece stands alone, and I'd rather have the argument for why out in the open than just hand over a diff. Happy for any part to be dropped.
Suggestion 1: let
dotnet packgenerate the package instead of the hand-written nuspecWhy.
src/AngleSharp.Js.nuspechas quietly drifted from what the project builds. It declares dependency groups fornet6.0andnet7.0, which aren't built, and has no group fornet10.0, which is shipped. Nothing is visibly broken — net10.0 consumers fall back to thenet8.0group — but the file has to be hand-edited every timeTargetFrameworkschanges, and it hasn't been.The benefit is that this class of drift stops being possible. The dependency groups get generated from
TargetFrameworks, so adding or removing a target updates the package automatically. Versions stop being duplicated between the csproj and the nuspec. And the.pdbfiles move out oflib/and into the.snupkg, which is where symbols are meant to live.Evidence, since swapping the packaging of a published library deserves more than assurances. I built the package on
develand on this branch and diffed them. Identical file listings. The only nuspec differences are the intended ones:net10.0added, deadnet6.0/net7.0removed,repositorynow carryingbranchandcommit, andowners/requireLicenseAcceptancedropped (both deprecated;requireLicenseAcceptancedefaults to the same value). Dependency ranges are preserved exactly —AngleSharp [1.5.0, 2.0.0),Jint [4.15.3, 5.0.0)— as is assembly identity:Version=1.0.0.0, PublicKeyToken=e83494dcdc6d31ea, same FileVersion, InformationalVersion,InternalsVisibleTokey andComVisible.One sharp edge worth flagging, because I walked into it. Pack publishes the reference version verbatim, so expressing AngleSharp as a float (
1.*) silently turned the published range into an unbounded>= 1.6.0and lost the2.0ceiling.AngleSharpVersionis now the floor, withDirectory.Packages.propsbuilding[$(AngleSharpVersion),2.0.0)around it — so the ceiling holds even when CI overrides the floor, andci.ymlneeded no change (it's GitBase-synced, so leaving it alone seemed right).Suggestion 2: drop
Microsoft.SourceLink.GitHubWhy. SourceLink has shipped in the .NET SDK since 8.0.100; the 8.0.0 package is a no-op there.
The benefit is one less dependency to track for zero behaviour change — the repository metadata still lands in the package, which the diff above confirms.
Suggestion 3: move the test project to net10.0, and untangle what it actually tests
Why.
net8.0goes out of support in November 2026. But the more interesting find: theProjectReferenceto the library was pinned to<TargetFramework>netstandard2.0</TargetFramework>, so every test ran against the netstandard2.0 build regardless of which framework the tests targeted. Thenet8.0andnet10.0library builds were compiled and never executed.The benefit is that tests now exercise the build they ship against, which is the whole point of multi-targeting. 243 tests pass on all three test targets.
The trade-off, stated plainly: the library still ships a
net8.0target, and with the test project onnet10.0only, that target is compiled but not tested. It wasn't tested before either, so this is not a regression — but if you'd prefernet8.0;net10.0on the test project, that's a one-line change and I'll make it.Suggestion 4: smaller consistency cleanups
Each of these is independent, and the benefit in every case is "one source of truth instead of two":
src/Directory.Packages.props). Versions were spread across two csproj files and the nuspec — which is how the nuspec drifted in the first place.ProjectConfigurationblocks removed from the.sln, for project GUIDs that aren't declared in the solution. (This started as a.slnxconversion; reverted in ce2bb37 per review — the format change wasn't worth narrowing the tooling contributors can use, so only the dead blocks go.)AssemblyInfo.csfiles folded into the csproj. They claimedCopyright © AngleSharp, 2013-2019while the nuspec said2017-2026; the assembly and package now agree.CHANGELOG.mdbecomes the single version authority. It was already parsed for the package version; it is now also passed to the build as-p:Version, so<Version>inDirectory.Build.propsis just the fallback for builds that bypass the NUKE script. (I first pinnedAssemblyVersionto1.0.0.0here to protect the strong-name identity — dropped in f6fbd93 after checking that published AngleSharp bumps it every minor, so the pin would have had the two packages following opposite policies. The built assembly is1.0.0.0either way.)ContinuousIntegrationBuildin CI, via.SetContinuousIntegrationBuild(IsServerBuild)innuke/Build.cs— the mechanism the core repo already uses. Without it the published symbols carry the build agent's absolute paths, which is why nuget.info flags beta.137 as non-deterministic.DelaySign=falseremoved. Conditioned on Windows, andfalseis the default everywhere — it never did anything.Withdrawn in c9db87e — the absence is deliberate and the repo should not restrict the SDK version. AGENTS.md now says so explicitly, since the line I read as describing a gap was actually describing a decision.global.jsonadded.NuGet.CommandLinedownload dropped now that pack and push both go throughdotnet.AGENTS.mdupdated where it describes things this changes.Verification
Clean Packageend to end with CI's exact invocation (-AngleSharpVersion 1.5.0): 243 tests pass onnet10.0,net462andnet472, package and symbol package produced, zero warnings withTreatWarningsAsErrorson.Deliberately not done
ClassicAssert, touching all 28 test files. That's a separate PR, if it's wanted at all.ci.ymlstill usesecho "::set-output name=...", which GitHub has disabled. It looks broken, but it's in the GitBase-synced set, so the fix belongs upstream rather than here.🤖 Generated with Claude Code