-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 support for non-string assembly attributes #2281
Comments
From @dasMulli on July 11, 2017 20:26 On Slack, a few ppl asked how to set |
I'm searching for |
(sorry, got distracted while writing this explanation of why this is the right place for this) This is the right home for this issue, because it's a feature request for the There's no way to do this currently. You can always add a standard Why do you want to do this with MSBuild instead of C#? I'm not totally opposed to the idea but would rate it as pretty low-priority. |
I'd like to do it with MSBuild for consistency with other attributes that we're defining. Also, the snippet you pointed at refers to |
You're right, I was pointing to the wrong implementation. The one you care about is in the Sdk. I'd be interested in seeing proposals for how to carry the metadata needed to make this possible. It's already very ugly with |
Team Triage: |
Sorry for being late to the party on this. I know it's closed, but I think it's worth considering reopening. In my opinion, this can be achieved very easily without breaking the existing behavior or requiring complex, brittle parameter magic. BackgroundMost of the attributes that used to be placed in code files (ex: AssemblyInfo) have been replaced with first-class equivalents in MSBuild 15.0, save a handful of outliers. To my knowledge the following attributes do not have built-in build properties in MSBuild 15.0:
The AssemblyTrademarkAttribute can use the existing AssemblyAttribute build items, but the others cannot because they are not of type While it's true that this limitation can be mitigated with a linked or dynamically generated code file, it would be far more preferable to have it supported as a first-class citizen. In large solutions, this information would typically be placed or imported into directory.build.props. Proposed SolutionTo enable this scenario, the existing implementation just needs to not wrap the provided value in double quotes. There currently is no validation on the input value matching the attributes because the compiler will handle that automatically; therefore, there is no need to include or otherwise formally support type semantics. To achieve this behavior without breaking the existing behavior, I propose you simply add one new, optional metadata element - IsLiteral (Boolean). The default value will be <ItemGroup>
<AssemblyAttribute Include="System.CLSCompliantAttribute" IsLiteral="true" _Parameter1="true" />
</ItemGroup> [assembly: System.CLSCompliant(true)] Figure 1: Using IsLiteral to omitted surrounding quotes <ItemGroup>
<AssemblyAttribute Include="System.Reflection.AssemblyTrademarkAttribute"
IsLiteral="true"
_Parameter1=""My Company"" />
</ItemGroup> [assembly: System.Reflection.AssemblyTrademarkAttribute("My Company")] Figure 2: Using IsLiteral to with user-defined double quotes BenefitsIn addition to these basic scenarios, this approach would enable more advanced scenarios without additional modification. Consider a scenario where a developer would specify the attribute for an OWIN startup class. Today, this is often in the assembly information file (*.cs, *.vb, etc) or it is placed in an arbitrary file (ex: Startup.cs). This new support would allow it to go into a *.props file or the *.*proj directly. <ItemGroup>
<AssemblyAttribute Include="Microsoft.Owin.OwinStartup"
IsLiteral="true"
_Parameter1="typeof(Microsoft.Examples.Startup)" />
</ItemGroup> [assembly: Microsoft.Owin.OwinStartup(typeof(Microsoft.Examples.Startup))] Figure 3: Using IsLiteral to insert an arbitrarily supported value such as a type |
I do like this suggestion. Doing it in msbuild may still be beneficial in situations where a Directory.Build.* file is easier to use than shared AssemblyInfo.cs files. I'd just argue that a name like |
There any number of possible names:
I'm good with any name if it lands. While I can certainly appreciate schedule, this shouldn't require a large development effort at all to implement (IMHO). If the team approves the feature and can agree on a suitable metadata item name, I get the impression there are multiple folks in the community that would submit a PR for this. |
Actually, this came up with us today. We would like to mark all of our sdk-style projects as ComVisible false, and adding an AssemblyInfo.cs to every project (even implicitly) instead of adding a property into a shared |
Proof-of-concept: master...dasMulli:feature/literal-code-fragment Will allow e.g.: <ItemGroup>
<AssemblyAttribute Include="System.Runtime.InteropServices.ComVisibleAttribute">
<_Parameter1>false</_Parameter1>
<_Treat_Parameter1AsLiteral>true</_Treat_Parameter1AsLiteral>
</AssemblyAttribute>
</ItemGroup> @rainersigwald does this seem reasonable? |
@dasMulli we'll see what @rainersigwald says. I think you've proven that this can work and isn't terribly complex. However, IMHO having an additional metadata item for every parameter is overkill. My proposal and continued position is that you only need a single metadata item (called Nice work putting this together. Hopefully that will grab the attention of the team and get them to roll it in. |
Happy to turn it into a proper PR whatever the decision. fyi I can see myself porting stuff to SDK-based csproj that needs [assembly: log4net.Config.XmlConfigurator(ConfigFileExtension="log4net",Watch=true)] for all executables, so a |
@dasMulli this looks great. I'm not sure what you mean about porting stuff to SDK-based projects and log4net, but I'd recommend making a minimal PR to increase its chances of getting merged. |
For what it's worth, I don't think introducing support for literals in the WriteCodeFragment task is coherent with the initial design. Let's not forget that the task allows a I do agree though that it would be convenient to use the task to write common boolean attributes such as ComVisible and CLSCompliant. AFAIK, MSBuild does recognize boolean expressions as such. Therefore, shouldn't the WriteCodeFragment task already have everything it needs to at least support both string and boolean parameters? Fixing just this would go a long way already, IMHO. |
One of the problems would be recognising when to use bools, strings or something else. You could define a custom attribute class in custom code and let For "literal arguments", I'd assume that the literal matches the language used. If you switch between VB and C#, you may need to change the literal arguments to |
Retorting to @AndyGerlicher's "won't fix" assessment
@dasMulli proved this is not at all complex
it's a real messy workaround when your solution has 143 projects and you try to nicely manage them with Directory.Build.targets |
Agreed. Though it may not be as nice since the proposed change couples the target files to the language used - even just for C# and VB there's a difference between The workaround at the moment would be to have some files |
@Crono1981 I couldn't disagree more with:
If it's not coherent, then the name of the task is horribly wrong. I'm going out on a limb here, but I think most people would interpret Write Code Fragment as just that. Granted, that doesn't mean the task can write any code, but it certainly doesn't say Write String Only Fragment, Write Attribute, or something like that. The expectation for the use of literals should be that of the author; otherwise, caveat emptor. If multi-targeting languages is something that is required, then obviously that's something the author needs to bake into their targets. As you suggest, using a language discriminator can easily be used to do this: <ItemGroup Condition=" '$(Language)' == 'C#' ">
<AssemblyAttribute Include="System.CLSCompliantAttribute" IsLiteral="true" _Parameter1="true" />
<AssemblyAttribute Include="System.ComVisibleAttribute" IsLiteral="true" _Parameter1="true" />
</ItemGroup>
<ItemGroup Condition=" '$(Language)' == 'VB' ">
<AssemblyAttribute Include="System.CLSCompliantAttribute" IsLiteral="true" _Parameter1="True" />
<AssemblyAttribute Include="System.ComVisibleAttribute" IsLiteral="true" _Parameter1="True" />
</ItemGroup> But even that is probably a non-issue because these items would already be split out into language-specific targets. @dasMulli you're right. There shouldn't be any interpretation of what the literal means. This would add unnecessary complexity. This complexity already exists to some degree because C# and VB have different string escaping rules. You can see this complexity in the task source code. Mistakes in the literal should the onus of the author. A possible compromise would be limiting literal values to specific types of primitives, which would be neutral across languages; for example, Boolean and numbers. You could fallback to the least common denominator as necessary. VB is not case-sensitive so @dasMulli, @AmadeusW the messiness is real. It can get even messier when you have additional variability. For example, some projects (the main source) are CLS-compliant and others (say tests) are not. You then end up with something in [assembly: ComVisible(false)]
#if CLS_COMPLIANT
[assembly: CLSCompliant(true)]
#endif And <ItemGroup>
<Compile Include="$(MSBuildThisFileDirectory)SharedAssemblyInfo.cs" Visible="false" />
</ItemGroup>
<PropertyGroup>
<ClsCompliant Condition=" '$(ClsCompliant)' == '' ">true</ClsCompliant>
<DefineConstants Condition=" '$(ClsCompliant)' == 'true' ">$(DefineConstants);CLS_COMPLIANT</DefineConstants>
</PropertyGroup> |
Why was this closed exactly? Is there a solution to this now? |
I think this should be reopened since it's still an issue. The only solution ATM would be to write a custom target to generate another Assembly.cs (see last line of this duplicate issue's comment: #3412 (comment)) |
Almost certainly. It's a shame that this capability isn't available just yet, as this was a feature I was looking into implementing to provide custom attributes to the AssemblyInfo.cs Hopefully this is reconsidered and implemented. |
I would at least like to see it put in a backlog, as this is still a requested feature, with no 'good' workarounds, as I would love remove my basic @AndyGerlicher @rainersigwald Is there anyway this could be reopened and re-triaged? The issue was closed over 2 years ago, and seems to have picked up more support from when it was opened originally. If not, would it be appropriate to open a new issue? EDIT: Here is a copy of the comment (from @AndyGerlicher) with the original triage review, and why it was originally closed:
|
Reopening to swap with #3412 This is more possible now than before because the non-CodeDOM path could be removed (CodeDOM is now available on Core). I still don't prioritize it very highly personally, and I don't understand most of the pushback above on having source files that include the attributes. You could condition adding that |
Why not |
I would like to see this feature too, to be able to add NUnit attribute to configure tests that can be run in parallell
|
I will add yet another voice in calling for this work to be done. Honestly, the current implementation feels like a poor design and a mistake. Enabling developers to add assembly attributes to the csproj with the current syntax and then magically quoting the values that are generated is... not great. Magical quoting is not good and is an impediment to this syntax doing what it ought to do: replacing the old AssemblyInfo.cs files with a more modern, centralized place to configure your project. Why should I need to have both? What is the point of having So here's another use case I don't see mentioned: the ParallelizeAttribute for unit testing. Its constructor doesn't even take parameters so you have to specify them like this: It would be really nice to be able to remove the AssemblyInfo files from our test projects and be able to specify this at the csproj level. But without having some functionality where the the Developers have been asking for this functionality for 3 years because the current implementation of |
With the current state of CA1416 emitting incorrect platform warnings if you have any attributes defined in .cs files, this becomes a bigger issue (for us anyway!) |
It is rather easy to write a target that gets fairly close: <PropertyGroup>
<_LiteralAssemblyFile Condition="'$(Language)' == 'C#'">$(IntermediateOutputPath)LiteralAssemblyAttributes.cs</_LiteralAssemblyFile>
<CoreCompileDependsOn>
AddLiteralAssemblyAttributes;
$(CoreCompileDependsOn);
</CoreCompileDependsOn>
</PropertyGroup>
<Target Name="AddLiteralAssemblyAttributes"
BeforeTargets="CoreCompile"
Condition="'$(Language)' == 'C#'"
Inputs="$(MSBuildAllProjects)"
Outputs="$(_LiteralAssemblyFile)">
<ItemGroup>
<Compile Include="$(_LiteralAssemblyFile)" />
</ItemGroup>
<ItemGroup>
<_LiteralAssemblyAttrDecls Include="// <autogenerated />" />
<_LiteralAssemblyAttrDecls Include="extern alias %(LiteralAssemblyAttributeExternAlias.Identity);"
Condition="'%(LiteralAssemblyAttributeExternAlias.Identity)' != ''" />
<_LiteralAssemblyAttrDecls Include="[assembly: %(LiteralAssemblyAttribute.Identity)(%(LiteralAssemblyAttribute._Parameters))]"
Condition="'%(LiteralAssemblyAttribute.Identity)' != ''" />
</ItemGroup>
<WriteLinesToFile File="$(_LiteralAssemblyFile)" Encoding="UTF-8" Lines="@(_LiteralAssemblyAttrDecls)" Overwrite="true" />
</Target> That implementation allows for any combination of attribute arguments, because it just drops the content of the For the specific case of <ItemGroup>
<LiteralAssemblyAttribute Include="global::System.CLSCompliantAttribute" Condition="'$(IsCLSCompliant)' != ''">
<_Parameters>$(IsCLSCompliant)</_Parameters>
</LiteralAssemblyAttribute>
</ItemGroup> If the Having something just like this, taking the entire parameter list, would be very useful to have in the SDK. Naturally, it would be specialized on language, but I don't think that the content of the parameter list is ever going to be very different between C# and VB, and most users are likely using only one or the other. I think its reasonable to just require users of this variant to specialize on the language currently being compiled. |
Behavior changes when comparing strings on .NET 5+: https://docs.microsoft.com/en-us/dotnet/standard/base-types/string-comparison-net-5-plus CA1014: Mark assemblies with CLSCompliantAttribute: (recomendação) https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1014 "Fix": dotnet/msbuild#2281 (comment)
I got here while trying to address the <NoWarn>$(NoWarn);CA1014</NoWarn> I'm not convinced that CA1014 is obsolete (see discussion). I would prefer an easy way to mark my assembly as CLS compliant, but if the cost of is maintaining a legacy |
I've just created the pull request #6285 that would allow parameter types to be specified. It's similar to what has been mentioned in here, with a couple of improvements:
Here are some examples: The original issue from @AmadeusW: <ItemGroup>
<AssemblyAttribute Include="System.CLSCompliantAttribute">
<_Parameter1>true</_Parameter1>
</AssemblyAttribute>
</ItemGroup> Generates: [assembly: System.CLSCompliantAttribute(true)] or <Assembly: System.CLSCompliantAttribute(True)>
<ItemGroup>
<AssemblyAttribute Include="Microsoft.Owin.OwinStartup">
<_Parameter1>Microsoft.Examples.Startup</_Parameter1>
<_Parameter1_TypeName>System.Type</_Parameter1_TypeName>
</AssemblyAttribute>
</ItemGroup> Generates: [assembly: Microsoft.Owin.OwinStartup(typeof(Microsoft.Examples.Startup))] or <Assembly: Microsoft.Owin.OwinStartup(GetType(Microsoft.Examples.Startup))> log4net example from @dasMulli: <ItemGroup>
<AssemblyAttribute Include="log4net.Config.XmlConfigurator">
<ConfigFileExtension>log4net</ConfigFileExtension> <!-- Parameters are still treated as strings by default -->
<Watch>True</Watch>
<Watch_TypeName>System.Boolean</Watch_TypeName>
</AssemblyAttribute>
</ItemGroup> Generates: [assembly: log4net.Config.XmlConfigurator(ConfigFileExtension="log4net", Watch=true)] or <Assembly: log4net.Config.XmlConfigurator(ConfigFileExtension:="log4net", Watch:=True)> nUnit example from @Socolin: <ItemGroup>
<AssemblyAttribute Include="NUnit.Framework.Parallelizable">
<_Parameter1>NUnit.Framework.ParallelScope.Fixtures</_Parameter1>
<_Parameter1_IsLiteral>true</_Parameter1_IsLiteral>
</AssemblyAttribute>
</ItemGroup> Generates: [assembly: NUnit.Framework.Parallelizable(NUnit.Framework.ParallelScope.Fixtures)] or <Assembly: NUnit.Framework.Parallelizable(NUnit.Framework.ParallelScope.Fixtures)> |
…6285) Fixes #2281 Context This change allows the WriteCodeFragment task to define assembly attributes that require parameters that are not of type System.String. For example, CSLCompliantAttribute can be generated with a parameter of true instead of "true". Changes Made Additional metadata can be defined on an AssemblyAttribute that specifies how to treat the parameters specified in the metadata. There are three different ways that the parameters can be treated. Infer the Type Without specifying any additional metadata, attributes that are defined in the mscorlib assembly (i.e. types that can be loaded via System.Type.GetType(string)) will have their parameter types inferred by finding the constructor where the parameter count matches the number of parameters specified in the metadata. For example, this: <ItemGroup> <AssemblyAttribute Include="CLSCompliantAttribute"> <_Parameter1>true</_Parameter1> </AssemblyAttribute> </ItemGroup> Will produce the code: [assembly: CLSCompliantAttribute(true)] For backward-compatibility, if the attribute cannot be found, or no matching constructor is found, the parameter is treated as a string. Declare the Type An additional metadata item can be used to specify the full name of the parameter type. To do this, add a metadata item that has the same name as the parameter with "_TypeName" appended to the end. For example, this: <ItemGroup> <AssemblyAttribute Include="TestAttribute"> <_Parameter1>True</_Parameter1> <_Parameter1_TypeName>System.Boolean</_Parameter1_TypeName> </AssemblyAttribute> </ItemGroup> Will produce the code: [assembly: TestAttribute(true)] This also works with named parameters: <ItemGroup> <AssemblyAttribute Include="TestAttribute"> <Foo>42</IdentifyLevel> <Foo_TypeName>System.Int32</Foo_TypeName> </AssemblyAttribute> </ItemGroup> [assembly: TestAttribute(42)] All types that can be used as attribute parameters are supported, except for arrays. For backward-compatibility, if a metadata item ends with "_TypeName", but there is no metadata item for the parameter with that name, then it will be treated as another named property. For example, this: <ItemGroup> <AssemblyAttribute Include="TestAttribute"> <Foo_TypeName>System.Int32</Foo_TypeName> </AssemblyAttribute> </ItemGroup> Would produce the code: [assembly: TestAttribute(Foo_TypeName="System.Int32")] Specify the Exact Code For cases where declaring the type is insufficient (such as when the parameter is an array), you can specify the exact that that will be generated for the parameter by adding metadata that has the same name as the parameter with "_IsLiteral" appended to the end. For example, this: <ItemGroup> <AssemblyAttribute Include="TestAttribute"> <_Parameter1>new int[] { 1, 3, 5 } /* odd numbers */</_Parameter1> <_Parameter1_IsLiteral>true</_Parameter1_IsLiteral> </AssemblyAttribute> </ItemGroup> Will produce the code: [assembly: TestAttribute(new int[] { 1, 3, 5 } /* odd numbers */)] The limitation with this is that the code you provide is language-specific. For example, the literal value in the metadata above will only work in C#. If you used that same metadata in a VB.NET project, you would receive a compiler error. This works with both positional and named parameters. As with the ..._TypeName metadata, if an ..._IsLiteral metadata does not have a corresponding parameter name, it will be treated as a named parameter for backward-compatibility. Mixed Parameter Behavior Because the additional metadata only applies to a specific parameter, you can choose to treat different parameters in different ways. For example, you can infer/use the default behavior for one parameter, specify the type for the second parameter and use a literal value for the third. For example: <ItemGroup> <AssemblyAttribute Include="TestAttribute"> <_Parameter1>This is a string</_Parameter1> <_Parameter2>42></Parameter2> <_Parameter2_TypeName>System.Int32</_Parameter2_TypeName> <_Parameter3>new int[] { 1 }</_Parameter3> <_Parameter3_IsLiteral>true</_Parameter3_IsLiteral> </AssemblyAttribute> </ItemGroup>
To the person attempting to convert:
The following would work (granted that you have reference to the PresentationFramework): <ItemGroup>
<AssemblyAttribute Include="System.Windows.ThemeInfo">
<_Parameter1>System.Windows.ResourceDictionaryLocation.None</_Parameter1>
<_Parameter1_IsLiteral>true</_Parameter1_IsLiteral>
<_Parameter1_TypeName>System.Windows.ResourceDictionaryLocation</_Parameter1_TypeName>
<_Parameter2>System.Windows.ResourceDictionaryLocation.SourceAssembly</_Parameter2>
<_Parameter2_IsLiteral>true</_Parameter2_IsLiteral>
<_Parameter2_TypeName>System.Windows.ResourceDictionaryLocation</_Parameter2_TypeName>
</AssemblyAttribute>
</ItemGroup> |
In fact, you can even leave out the <ItemGroup>
<AssemblyAttribute Include="System.Windows.ThemeInfo">
<_Parameter1>System.Windows.ResourceDictionaryLocation.None</_Parameter1>
<_Parameter1_IsLiteral>true</_Parameter1_IsLiteral>
<_Parameter2>System.Windows.ResourceDictionaryLocation.SourceAssembly</_Parameter2>
<_Parameter2_IsLiteral>true</_Parameter2_IsLiteral>
</AssemblyAttribute>
</ItemGroup> |
Yes, thanks for pointing it out. |
Using MSBuild variables in parameters, seems to always trigger this warning now. Specifying
Anything I'm missing here? |
To infer the parameter type, MSBuild needs to know about the attribute without having to load any external assemblies. That means parameter inferencing is limited to attributes in the |
Understood, makes sense. Is there a way to supress that warning though, it doesn't seem to have a |
It's only logged as a message, so it doesn't affect the build in any way, but if you want to stop it from appearing in the build log, you can specify the parameter type: <ItemGroup>
<AssemblyAttribute Include="System.Reflection.AssemblyCommitDateAttribute">
<_Parameter1>$(CommitDate)</_Parameter1>
<_Parameter1_TypeName>System.String</_Parameter1_TypeName>
</AssemblyAttribute>
</ItemGroup> |
I tried that, but the message does not go away. |
That's strange. When I add the parameter type the message goes away. I tried it on both v16.11.0.36601 and v17.0.0.45303. |
Agree there should be better comprehension available to specify actual types during build. Leaving authors to have to get very creative in how they present their argument lists for sure. |
I think When |
Is there a way to set the module attribute via MSBuild? [module: SkipLocalsInit] |
@HavenDV, WriteCodeFragment does not support module attributes, and they are out of scope for this issue. Please see dotnet/sdk#24894 for generating SkipLocalsInitAttribute specifically. |
From @AmadeusW on July 11, 2017 16:44
There should be a way to generate non-string assembly attributes.
For example,
generates
which produces an error, because the parameter must be a boolean.
Copied from original issue: dotnet/sdk#1402
The text was updated successfully, but these errors were encountered: