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

Add Humanizer.Core #1895

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Add Humanizer.Core #1895

merged 1 commit into from
Nov 19, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Nov 17, 2020

This is https://github.com/Humanizr/Humanizer (different from https://github.com/Humanizer/Humanizer !)

Humanizer shows up as a dependency of ASP.NET Core and roslyn-analyzers. This tries to fix that. Supposedly it also ships with the SDK.

Fixes: #1767

@omajid omajid marked this pull request as ready for review November 17, 2020 19:28
@omajid omajid force-pushed the add-humanizer branch 2 times, most recently from e7312b3 to 9038fcb Compare November 17, 2020 19:49
@omajid omajid closed this Nov 17, 2020
@omajid omajid reopened this Nov 17, 2020
@omajid omajid changed the title WIP: add humanizer Add Humanizer.Core Nov 17, 2020
@dagood
Copy link
Member

dagood commented Nov 18, 2020

Looks like it isn't quite building the .Core package (the prebuilt is still there):

  [00:52:11.33] Building 'humanizer'...done
  New NuGet package(s) after building humanizer:
    -> Humanizer 2.2.0
  roslyn using package version properties saved at /src/artifacts/obj/x64/Release/PackageVersions.props.pre.roslyn.xml

I guess we need to pack https://github.com/Humanizr/Humanizer/blob/v2.2/NuSpecs/Humanizer.Core.nuspec in particular... looks like it's normally done directly by the repo's CI with nuget.exe: https://github.com/Humanizr/Humanizer/blob/v2.2/appveyor.yml#L33. 😕

I'm not familiar with a good way to do something like nuget pack--not sure if dotnet nuget pack or something works at this point. If it ends up with the same content, maybe we can just set the package id to Humanizer.Core in the current csproj and keep repos/humanizer.proj the same. /cc @dseefeld @crummel

@omajid
Copy link
Member Author

omajid commented Nov 18, 2020

Thanks, I was able to put together a dotnet pack incantation to build using the .nuspec file.

Now that I see that Humanizer.Core packages are produced:

./packages/restored/humanizer.core/2.2.0/humanizer.core.2.2.0.nupkg
./artifacts/src/Humanizer.b30550eed103a6970d8465fe7c5c16300b70be81/src/Humanizer/bin/Release/Humanizer.Core.2.2.0.nupkg
./artifacts/obj/x64/Release/blob-feed/packages/Humanizer.Core.2.2.0.nupkg

Strangely, as you can see in the output, something in roslyn downloads Humanizer.Core 2.2.0 again, even though it's in the blob feed.

Edit: maybe it's dotnet/roslyn#47290 ?

@dagood
Copy link
Member

dagood commented Nov 18, 2020

Nice, I think that command seems reasonable.

I'm not quite following what's pointing to a download happening. If it's about the nupkg in packages/restored/, that seems normal to me, NuGet restoring Humanizer and placing a copy of the restored nupkg in the package cache (whether downloaded or from a local source).

I don't really know why NuGet always does that, but it's convenient for us. We use it to do a check for this exact problem. 😄 We look for any package in the package restore cache that has the same id/version as a nupkg in the source-built blob feed. If there are any, we make sure it's byte-for-byte identical, as proof that the source-built version of the package is what ended up being used: https://github.com/dotnet/source-build/blob/master/tools-local/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/GetSourceBuiltNupkgCacheConflicts.cs. I'd expect that to throw a decently clear warning if you have a restore happening from the wrong feed here.

@omajid
Copy link
Member Author

omajid commented Nov 19, 2020

I'm not quite following what's pointing to a download happening. If it's about the nupkg in packages/restored/, that seems normal to me

Oh. I see. Thanks, that clears things up. The nuget packages are indeed bit-by-bit identical:

$ sha256sum $(find -iname 'humanizer*nupkg')
2255dc5c64cd27ca8d926974801fec0acff7a4ca219d99d2dbd013f093b54518  ./packages/restored/humanizer.core/2.2.0/humanizer.core.2.2.0.nupkg
2255dc5c64cd27ca8d926974801fec0acff7a4ca219d99d2dbd013f093b54518  ./artifacts/src/Humanizer.b30550eed103a6970d8465fe7c5c16300b70be81/src/Humanizer/bin/Release/Humanizer.Core.2.2.0.nupkg
2255dc5c64cd27ca8d926974801fec0acff7a4ca219d99d2dbd013f093b54518  ./artifacts/obj/x64/Release/blob-feed/packages/Humanizer.Core.2.2.0.nupkg

WorkingDirectory="$(ProjectDirectory)"
IgnoreStandardErrorWarningFormat="true" />

<Exec Command="$(DotnetToolCommand) pack $(ProjectDirectory)src/Humanizer/Humanizer.csproj /p:NuspecFile=$(ProjectDirectory)NuSpecs/Humanizer.Core.nuspec /p:NuspecBasePath=$(ProjectDirectory)src/ /p:NuspecProperties=Version=2.2.0 /p:Configuration=$(Configuration)"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to pull the Version value from eng/Version.Details.xml and use it here instead of hardcoding 2.2.0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add <HumanizerCorePackageVersion>2.2.0</HumanizerCorePackageVersion> to eng/Versions.props and use it from there. If Humanizer.Core ever gets updated with auto-update, this value will update as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That would result in the version being in 2 places: eng/Versions.props and eng/Version.Details.xml. Is there some way to use the value from Versions.props in Version.Details.xml?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. That's the pattern for ProdCon. When these files are auto-updated, the version and sha get updated in Version.Details.xml and the corresponding version tag gets updated in Versions.props.

@omajid omajid force-pushed the add-humanizer branch 2 times, most recently from 0de1500 to 2f27b3a Compare November 19, 2020 17:30
Comment on lines 10 to 11
<HumanizerCorePackageVersion>2.2.0</HumanizerCorePackageVersion>
<PrivateSourceBuildReferencePackagesPackageVersion>1.0.0-beta.20562.2</PrivateSourceBuildReferencePackagesPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict with adjacent line.

(I think it would be reasonable to put this in a new propertygroup just to force Git to ignore this in the future. Adjacency doesn't seem to me to be a very useful heuristic for meaningful conflicts in this file. 😛)

This is https://github.com/Humanizr/Humanizer (different from
https://github.com/Humanizer/Humanizer !)

Humanizer shows up as a dependency of ASP.NET Core and roslyn-analyzers.
This tries to fix that. Supposedly it also ships with the SDK.

Fixes: dotnet#1767
@dagood dagood merged commit 3be0c72 into dotnet:master Nov 19, 2020
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.

3 participants