stride icon indicating copy to clipboard operation
stride copied to clipboard
verified publisher icon stride3d

Problematic use of shared projects and code files

Open jrinker03 opened this issue 3 years ago • 5 comments

Release Type: GitHub

Version: master

Describe the bug The way in which multiple assemblies are reusing the same code files and shared projects within the same application leads to multiple duplicated symbols resulting in compiler warnings, slower compilation times and marginally larger assemblies.

In the case of shared projects (ex. Stride.NuGetResolver), some are not being used in the way .shproj were intended. Shared projects are for code and assets being reused across different applications, typically targeting different native platforms. More on that from Microsoft. A reasonable use in the codebase is Stride.Core.Assets.Yaml.shproj which is used by Stride.GameStudio and Stride.Launcher apps. Though a common assembly would have worked as well and avoided a recompile of the same code when building both apps. The code within the existing shared projects consumed in the same application should stand alone in their own assemblies or get rolled into one or more existing assemblies to eliminate the duplication issue.

Similarly, there are instances of individual .cs files being compiled into different assemblies which interact with each other in the same application. The largest offender is the SharedAssemblyInfo, which anyone who has compiled Stride.sln will have seen the many duplicate symbol warnings.

To Reproduce Compile Stride.sln and observe many warnings of these type (the assemblies in the warnings will differ per build):

warning CS0436: The type 'StrideVersion' in 'H:\src\jrinker03\stride\sources\editor\Stride.GameStudio\..\..\shared\SharedAssemblyInfo.cs' conflicts with the imported type 'StrideVersion' in 'Stride.Core.Assets.Editor, Version=4.1.0.1, Culture=neutral, PublicKeyToken=null'. Using the type defined in 'H:\src\jrinker03\stride\sources\editor\Stride.GameStudio\..\..\shared\SharedAssemblyInfo.cs'.

warning CS0436: The type 'PublicKeys' in 'H:\src\jrinker03\stride\sources\editor\Stride.Editor\..\..\shared\SharedAssemblyInfo.cs' conflicts with the imported type 'PublicKeys' in 'Stride.Engine, Version=4.1.0.1, Culture=neutral, PublicKeyToken=null'. Using the type defined in 'H:\src\jrinker03\stride\sources\editor\Stride.Editor\..\..\shared\SharedAssemblyInfo.cs'.

warning CS0436: The type 'SplashScreenWindow' in 'H:\src\jrinker03\stride\sources\shared\Stride.NuGetResolver\SplashScreenWindow.xaml.cs' conflicts with the imported type 'SplashScreenWindow' in 'Stride.ConnectionRouter, Version=4.1.0.1, Culture=neutral, PublicKeyToken=null'. Using the type defined in 'H:\src\jrinker03\stride\sources\shared\Stride.NuGetResolver\SplashScreenWindow.xaml.cs'.

Expected behavior If architected correctly, the code should have no duplicated code or compiler warnings.

jrinker03 avatar May 02 '22 20:05 jrinker03

First, some general remarks about shared projects:

Even if I agree that we don't follow typical pattern of multiplatform app for shared projects, our use still make sense. We use them simply to share a bunch of code files across multiple projects (before we were simply adding files as reference but it was difficult to maintain files one by one rather than as a group that is easy to edit within VS). So let's just consider shared code or shared project is used for the same purpose inside Stride.

Anyway, I would argue that the problem is not the shared project itself (which can save lot of duplication or avoid some unwanted dependencies) but the need to setup proper rules for how/when to use shared code.

Reasons and rules for shared code

I think we had several use cases:

  • share some code when some projects couldn't depend on Stride assemblies. Example: assembly processor, VS plugin
  • share code between several projects that are quite orthogonal (wanted to avoid creating yet another .dll/library for a single file used by 2 random projects, esp. when those files don't belong well to a shared assembly referenced by the project using it or if there is no such commonly referenced assembly yet)

However in many cases it's possible the assumptions at the time it was created are not true anymore (i.e. there is now a common library used by those two projects where it would make sense to have those libs, or the library that couldn't depend on Stride now can).

Anyway, as a general rule to avoid such warning, if a project is included in multiple assemblies, it should either be:

  • included only in projects that can't be referenced together (i.e. 2 projects that are totally orthogonal/independent and never used together)
  • otherwise, it should only be private code (or it might clash if included twice through different projects)

Concerning SharedAssemblyInfo.cs:

I think we usually respect those 2 rules for most shared code, but SharedAssemblyInfo.cs is currently not doing it perfectly because it has some classes which, while not public (worst case), are internal (instead of private) so causing those warning.

Here's some potential solutins:

  • We could try to switch PublicKeys and StrideVersion to private again (not sure about why we switched from private to internal at some point, I remember there was a reason but maybe it's not necessary anymore?)
  • Another option would be to split the file in two: assembly attributes (i.e. AssemblyVersion) which need to be for every assembly, from StrideVersion + PublicKeys class (only in Stride.Core)? BTW AssemblyAttribute can now be defined by csproj attributes (but it will need some changes to version parsing done in a few places).
  • Otherwise being internal it's not a big deal as we know that all classes have the same data within same compilation of Stride. we could put a pragma warning to remove CS0436 just for this class?

xen2 avatar May 03 '22 12:05 xen2

And just to be clear: Many of those shared project might have been done when project was structured quite differently, so I am sure a lot of them don't make sense anymore or can be done in a much better way. So it's of course totally fine to review some of them over time. But if target is to fix that SharedAssemblyInfo.cs warning I think it's quite a specific case that need special handling (cf previous post).

xen2 avatar May 03 '22 12:05 xen2

Thanks, xen2. I was hoping you would comment and glad you had the time to do so.

I would argue that the problem is not the shared project itself (which can save lot of duplication or avoid some unwanted dependencies) but the need to setup proper rules for how/when to use shared code.

Yes, we're in agreement. The intent of this issue is not to disparage shared code. It's to ultimately try to get to a clean and well-maintained codebase where both new and existing developers find it a pleasure to work in. Probably no need for me to go into why that's a really good thing.

The only feedback I have on your Reasons and Rules is the second use case has the potential for creating technical debt (and a former mentor of mine would argue it is technical debt) so I'd love to see some good discussion around such cases in the future.

However in many cases it's possible the assumptions at the time it was created are not true anymore

The good news is I'm finding this to be the case, so far. We may wind up putting more energy in discussing the topic than the actual changes. ;)

Concerning SharedAssemblyInfo.cs

Let me see if making them private does the job and, if not, what it might take to get there.

Yes, I'd prefer to take advantage of new capabilities in MSBUILD and possibly TeamCity but, as you say, that can be done over time. I'm all for taking the potentially simpler, short-term improvement while continuing to discuss the next evolution.

jrinker03 avatar May 03 '22 16:05 jrinker03

Sadly, the assembly attributes at the top of SharedAssemblyInfo.cs need the classes to be not-private to work as the code is written.

jrinker03 avatar May 04 '22 01:05 jrinker03

Sorry if not clear, what I meant is that those [assembly: AssemblyVersion(xxx)] attributes are private and need to be specified for each assembly. If left as code, the shared file makes sense. However, we could do it directly in the .csproj with the new .NET build system (esp. since we switched 100% .NET 5+ recently): https://docs.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#nuget-metadata-properties This is not as trivial as just adding them in some shared .props file: some other part of the build system which was parsing version from SharedAssemblyInfo.cs needs to be changed to directly use the properties.

Going back to the main point, my discussion about private/public/internal was only for the StrideVersion/PublicKeys classes.

Another point: we don't do .NET strong name signing anymore (only standard .exe/dll code signing). So PublicKeys class and STRIDE_SIGNED define is not necessary anymore.

xen2 avatar May 05 '22 13:05 xen2