Make it easier to opt into "Core" satellite assembly gen #2726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently the `GenerateSatelliteAssemblies` target handles generating satellite assemblies, _unless_ we're using the .NET Core version of MSBuild. In this case we skip `GenerateSatelliteAssemblies` and this is instead handled by the `CoreGenerateSatelliteAssemblies` target in the .NET SDK. This is because `GenerateSatelliteAssemblies` utilizes alink (al.exe) which has no counterpart for .NET Core. Instead, `CoreGenerateSatelliteAssemblies` uses csc.exe to create the assembly. There are times where it would be helpful to use the .NET Core approach, even when we aren't using the .NET Core MSBuild. For example, al.exe has some key limitations. For example, it has [no support for public-signing](dotnet/roslyn#23191) and [poor handling of AssemblyInformationalVersionAttribute](dotnet/roslyn#23190). These issues are currently blocking the build of the dotnet/roslyn repo from generating its own satellite assemblies. Opting _into_ `CoreGenerateSatelliteAssemblies` is easily done by setting the `GenerateSatelliteAssembliesForCore` property to `true`. However, there is currently no good way to opt _out_ of `GenerateSatelliteAssemblies`. This commit replaces the broad check that we're not using the "Core" runtime type with a specific check that `GenerateSatelliteAssembliesForCore` is not set to `true`.
nguerrera
approved these changes
Nov 16, 2017
cdmihai
approved these changes
Nov 16, 2017
This was referenced Nov 17, 2017
|
Why isn't this the default? |
|
@tmat Good question @tmeschter Does the AL based satellite generation have any features not present in the csc based satellite generation (the much longer argument list suggests that it does more ...)? If not, should we just replace GenerateSatelliteAssemblies with CoreGenerateSatelliteAssemblies? I do not see any back compat issues, but I am also not very intimately familiar with this. |
|
Except for a very limited set of consumers (currently just dotnet/roslyn) there's no value in using CoreGenerateSatelliteAssemblies over GenerateSatelliteAssemblies. There will almost certainly be back-compat issues (example: until yesterday CoreGenerateSatelliteAssemblies only supported C#). At some point making it the default might be the right thing to do, but not right now. |
tmeschter
added a commit
to tmeschter/roslyn
that referenced
this pull request
Dec 5, 2017
The standard `GenerateSatelliteAssemblies` target that ships with MSBuild uses al.exe (alink) to create satellite assemblies. However, there are a couple of things it doesn't support that we need in the dotnet/roslyn repo--see issues dotnet#23190 and dotnet#23191 for details. Instead, we want to use the `CoreGenerateSatelliteAssemblies` target from Microsoft.NET.Sdk. We've already opted in to that target, and now we need to opt out of the original `GenerateSatelliteAssemblies`. Until we can move to a copy of MSBuild that includes the change in dotnet/msbuild#2726 the best way to do that is to redefine the target as a no-op.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the
GenerateSatelliteAssembliestarget handles generating satellite assemblies, unless we're using the .NET Core version of MSBuild. In this case we skipGenerateSatelliteAssembliesand this is instead handled by theCoreGenerateSatelliteAssembliestarget in the .NET SDK. This is becauseGenerateSatelliteAssembliesutilizes alink (al.exe) which has no counterpart for .NET Core. Instead,CoreGenerateSatelliteAssembliesuses csc.exe to create the assembly.There are times where it would be helpful to use the .NET Core approach, even when we aren't using the .NET Core MSBuild. For example, al.exe has some key limitations. For example, it has no support for public-signing and poor handling of AssemblyInformationalVersionAttribute. These issues are currently blocking the build of the dotnet/roslyn repo from generating its own satellite assemblies.
Opting into
CoreGenerateSatelliteAssembliesis easily done by setting theGenerateSatelliteAssembliesForCoreproperty totrue. However, there is currently no good way to opt out ofGenerateSatelliteAssemblies. This commit replaces the broad check that we're not using the "Core" runtime type with a specific check thatGenerateSatelliteAssembliesForCoreis not set totrue.