Consider adding Humanizer to source-build #1767
Comments
crummel
added
area-product-experience
dseefeld
added
triaged
area-patch-removal
Note: Removed aspnetcore version number patch because it is set to a stable 5.0 version. Note: Removed roslyn Humanizer removal patch because there is now more usage of Humanizer in the code base. May need to redo this patch or add Humanizer. (See dotnet#1767)
|
For 5.0 GA, #1845 removes the patch that had removed the Humanizer prebuilt. Humanizer got a lot more usage in GA and it wasn't reasonable to try to reapply it all while trying to get GA through. We'll either need to recreate the patch or add the repo. /cc @dseefeld The repo looks like a normal-enough |
Note: Removed aspnetcore version number patch because it is set to a stable 5.0 version. Note: Removed roslyn Humanizer removal patch because there is now more usage of Humanizer in the code base. May need to redo this patch or add Humanizer. (See dotnet#1767)
|
Humanizer is also used by roslyn-analyzers |
|
The Microsoft SDK doesn't include Humanzier, does it? Is this shipped with the SDK? |
|
Huh... maybe none of the projects (as in csproj) that build using Humanizer actually ship in the .NET SDK? Considering we had to actually patch If we can disable unused projects, that'd be ideal. If for some reason that doesn't work yet we do know the projects aren't used, we could put Humanizer in SBRP. @crummel, do you know more about this, since you implemented the first patch? The last possibility I can think of is Humanizer being used at build-time but not in the SDK, somehow. I don't have any evidence pointing to it. If we disable the projects or put Humanizer as a ref package, it seems more likely that we'll see a build break rather than something subtle (a string breaking somewhere in the SDK), so I don't think this is a big concern. |
|
I'm pretty sure Microsoft.CodeAnalysis.CSharp.Workspaces should at least be shipping, the other code style projects maybe not. I'm taking a look now. |
|
I tried building roslyn after removing the
The RC2 SDK includes |
|
It looks like Roslyn removed the references from a lot of projects except
I'm seeing kind of the opposite in terms of the DLLs being in the SDK. Nothing for Which would be a good thing for us. Maybe |
|
@333fred, can you help with this question for 5.0 roslyn source-buildability? Is |
Yes, it is. In fact, dotnet/roslyn#48999 just added the CodeStyle layer because that is shipped in source-build, and that has an explicit dependency on the workspaces layer. |
|
@333fred Interesting, when/where do you expect |
|
@333fred functionally roslyn doesn't need Humanizer, right? Would we want to consider removing it from roslyn all together for .NET 6? |
I'm not sure; the question was whether the Workspaces layer is in source-build, and that should be true.
The compiler does not, but many of our analyzers do. For example, @dagood you mentioned the other day that you were trying to get the roslyn-analyzers repo into source-build, and that has a dependency on humanizer as well: https://github.com/dotnet/roslyn-analyzers/blob/79c2638f54b9e7ef0a3954cc838ec4c13ea63c1f/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Microsoft.CodeQuality.Analyzers.csproj#L15, which it uses for CA1714. It's possible that the IDE could move the humanizer dependency out of the Workspaces layer (I couldn't find a reference to the extension methods actually in the workspaces layer, mostly in the features layer), but I'm not sure whether that would actually get rid of the dependency given that roslyn-analyzers ship in-box in .NET 5 and we'd like to be able to source-build them as well. |
That's not what I meant by my question--when I say the SDK here I mean the .NET SDK:
(Even if something's built in source-build, that doesn't necessarily mean it's shipped in a .NET SDK, source-built or not. For example, we build dotnet/arcade.) @jaredpar you're also listed as a Roslyn champ, maybe you can help figure this out? |
|
@dagood what question do we need resolved here? |
|
Two questions:
|
|
@dagood before that though, I do think it's important to address this question:
I can't exclude workspaces from source-build, particularly as we just added codestyle to source-build. And I don't know whether moving the Humanizer dependency from the workspaces layer to a higher layer would help, because other analyzers depend on humanizer and since they ship in-box with .NET 5, I expect they're in the SDK. Looking at the SDK, I see the analyzers shipped in-box are
So,
Well, I'm surprised Workspaces isn't showing up in the SDK. And I'm therefore surprised that the dependencies of Workspaces aren't showing up in the SDK. I think moving the Humanizer dlls to the features layer would be possible: I think all the functionality that depends on it is actually in the features layer, not the workspaces layer. |
|
I have a question that's mostly unrelated to the issue at hand. I see terms like "workspaces" and "codestyle", and I dont know what they mean. Is there some document I can read up on that clarifies what these components are and which of these are shipped (or should be shipped) as part of the .NET SDK? |
These are roslyn feature layers. There's no real document that I know of, but here's the basics: For every layer, there is a non-language-specific core, and 2 language specific implementations. Layer 1: The actual compiler dlls. These are responsible for actually compiling code, and exposing APIs for analyzers to use. The CodeStyle project is a set of analyzers that have a dependency on Layer 2 here, and they ship in the SDK. The .NET 5 in-box analyzers also have a dependency on layer 2. |
|
Thanks, a lot of answers. 😄 I don't have any objections to adding Humanizer to source-build and it seemed like a reasonable dependency--just takes some work since it's not on the usual infra. (Maybe since it's now in .NET Foundation we will be able to work with the maintainers in the future to add arcade + arcade-powered source-build? 🤞) Removing it is interesting, but it sounds like we're beyond being able to do that for 5.0 without losing features vs. the Microsoft-built SDK. (Which is my focus.) My main concern here is how weird and confusing it is, how some DLLs aren't showing up where we expect. I wonder if this rises to the level of something to investigate with the Microsoft-built .NET SDK build as well? For now I guess we can assume that as long as the source-built .NET SDK has similar files, we'll at least have parity. On that note... here's a link to the current 5.0 SDK we're targeting for source-build 5.0 GA #1845: |
* Update to 5.0.100-rtm.20521.5, BAR Build ID 68494 * Update previously source-built to N-1 * Reconcile patches Note: Removed aspnetcore version number patch because it is set to a stable 5.0 version. Note: Removed roslyn Humanizer removal patch because there is now more usage of Humanizer in the code base. May need to redo this patch or add Humanizer. (See #1767) * Update to 5.0.100-rtm.20522.4, BAR Build ID 68583 * Update commits in Version.Details.xml and reconcile patches * Remove temporary RC1 fix in aspnetcore * Install built runtime for usage by aspnetcore * Update roslyn to build CodeStyle packages * Update / add installer patches * Fix broken patch * Update prevSourceBuilt to GA version * Pin System.Security.Cryptography.Cng version in runtime build * Update path to sdk installation * Update smoke test nuget.config entries * Update online prebuilt baseline * Update package name for aspnetcore reference * Add runtime feed to smoke-testNuGet.Config * Update runtime config entry * Update to 20201026.5, BAR Build ID 68880 * Set UseStableVersions flag * Update online baseline * Update offline prebuilt baseline * Include patch from #1850 Include patch to remove import of restore/harvestPackages.targets in runtime libraries build to fix issue with portable build * Re-add portable build only prebuilts
|
Circling back since roslyn-analyzers was recently added: it looks like only one part of roslyn-analzyers needs Humanizer. And that part of roslyn-analzyers is not used by source-build. |
|
We still need it for Roslyn though, right? @omajid are you still planning to work on this? |
I am still a little confused as to why it's not shipped as part of the Microsoft SDK ...
If we (still) need it in source-build, sure, I can look into this. |
@333fred have you looked into this?
We could throw it in SBRP if it's only compiled against and never executed, I guess. Since it's not shipping in the SDK that seems possible. We weren't able to patch out usages in the 5.0.0 GA PR (#1767 (comment)) so it doesn't seem worthwhile to try that approach again. |
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
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
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
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
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
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
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
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
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
|
Fixed with #1895 |
* Update to 5.0.100-rtm.20521.5, BAR Build ID 68494 * Update previously source-built to N-1 * Reconcile patches Note: Removed aspnetcore version number patch because it is set to a stable 5.0 version. Note: Removed roslyn Humanizer removal patch because there is now more usage of Humanizer in the code base. May need to redo this patch or add Humanizer. (See dotnet/source-build#1767) * Update to 5.0.100-rtm.20522.4, BAR Build ID 68583 * Update commits in Version.Details.xml and reconcile patches * Remove temporary RC1 fix in aspnetcore * Install built runtime for usage by aspnetcore * Update roslyn to build CodeStyle packages * Update / add installer patches * Fix broken patch * Update prevSourceBuilt to GA version * Pin System.Security.Cryptography.Cng version in runtime build * Update path to sdk installation * Update smoke test nuget.config entries * Update online prebuilt baseline * Update package name for aspnetcore reference * Add runtime feed to smoke-testNuGet.Config * Update runtime config entry * Update to 20201026.5, BAR Build ID 68880 * Set UseStableVersions flag * Update online baseline * Update offline prebuilt baseline * Include patch from dotnet/source-build#1850 Include patch to remove import of restore/harvestPackages.targets in runtime libraries build to fix issue with portable build * Re-add portable build only prebuilts
Roslyn uses Humanizer to pluralize and convert to/from CamelCase for human-friendly automatic variable names and debug output. This was removed in #1766 to remove the prebuilt. We should either work with Roslyn to eliminate this dependency or add the Humanizer repo to source-build.
The text was updated successfully, but these errors were encountered: