Add releaser config. Remove AssemblyInfo and move InternalsVisibileTo to the csproj. #5
Conversation
|
This pull request has been linked to Shortcut Story #169461: Create a releaser config for the dotnet open feature provider.. |
.ldrelease/config.yml
Outdated
| version: 2 | ||
|
|
||
| repo: | ||
| public: openfeature-dotnet-server |
There was a problem hiding this comment.
You don't need to specify repo at all if there's only one repo. Its only purpose is for setting up a public/private relationship.
There was a problem hiding this comment.
Cool. Will remove that.
.ldrelease/config.yml
Outdated
| template: | ||
| name: dotnet6-linux | ||
| env: | ||
| LD_RELEASE_DOCS_ASSEMBLIES: LaunchDarkly.OpenFeature.ServerProvider |
There was a problem hiding this comment.
You shouldn't need to set LD_RELEASE_DOCS_ASSEMBLIES unless you need to document something in addition to, or other than, the assembly you're building.
| <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo"> | ||
| <_Parameter1>LaunchDarkly.OpenFeature.ServerProvider.Tests</_Parameter1> | ||
| </AssemblyAttribute> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Could you say more about the reason for doing this instead of using AssemblyInfo.cs?
Besides being (IMO) somewhat harder to read due to the _Parameter1 thing, I think there's a probably unintentional change in behavior here: now it is always setting this property, instead of setting it only in debug builds. In our other projects, we only set InternalsVisibleTo in debug builds. Not only is there no good reason to have it in the release build we publish, but also, I think it may actually make a release build fail because, if we are signing the LaunchDarkly.OpenFeature.ServerProvider assembly (as we normally do for all of our release builds), then it will expect LaunchDarkly.OpenFeature.ServerProvider.Tests to be signed too.
There was a problem hiding this comment.
I will see if I can set it in debug. Doing something like Condition="'$(Configuration)' == 'Debug'" or inverted != Release.
I was moving it because it seemed like assemblyinfo was not really the way people were doing much anymore. Once they added automatic generation at build time of the assemblyinfo as the default.
So, just trying to be modern. But if you feel strongly, I really could not care, and I can move it back.
There was a problem hiding this comment.
I am also kind of curious what the strong name ecosystem is like now, being as it seems to be changing.
https://learn.microsoft.com/en-us/dotnet/standard/assembly/strong-named
For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.
If you are an open-source developer and you want the identity benefits of a strong-named assembly for better compatibility with .NET Framework, consider checking in the private key associated with an assembly to your source control system.
There was a problem hiding this comment.
Not to say we don't. Just interesting documentation.
There was a problem hiding this comment.
Made the ItemGroup conditional.
There was a problem hiding this comment.
Looks like there is a nicer method: dotnet/sdk#3439
That would limit to building with .Net 5 and newer.
There was a problem hiding this comment.
You could still target net471. Do we have any stance on the build tools supported?
<ItemGroup Condition="'$(Configuration)'!='Release'">
<InternalsVisibleTo Include="LaunchDarkly.OpenFeature.ServerProvider.Tests" />
</ItemGroup>
Sure looks a lot nicer.
There was a problem hiding this comment.
Well, "build tools supported" in this case would mean the build tools used to build this library, right? I think developers could still consume the NuGet package we built if they were, for instance, using .NET Core 3.1 tools to build their app and using the .NET Standard 2.0 version of our library. I don't think our platform support/EOL policy is necessarily meant to apply to building our projects from scratch with older tools... although that's a bit of a gray area that we haven't explicitly addressed, so I'll bring it up to the team for discussion.
There was a problem hiding this comment.
Updated with the cleaner version.
There was a problem hiding this comment.
As for whether it's worthwhile to do strong-naming, that's been an ongoing and inconclusive discussion for a long time. It never made any difference in .NET Core, and it still doesn't, and .NET 5+ is equivalent to .NET Core in that regard. It was a thing in .NET Framework, and still is; whether it's still a thing that any of our customers are using, we don't know. But if they are using it for any reason, then it's always possible that they have a build-time policy forbidding any non-strong-named dependencies, so for us to stop doing this could be a breaking change (again, only for .NET Framework).
No description provided.