Change in AssemblyCommon.props to simplify signing and support project level stylecop.json #943
Conversation
…hub.com/AndreyTretyak/qsharp-compiler into andreyt/custom-stylecop-config-for-llvm
…ylecop-config-for-llvm # Conflicts: # src/QsCompiler/LlvmBindings/LlvmBindings.csproj # src/QsCompiler/QirGeneration/QirGeneration.csproj
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
anpaz
left a comment
anpaz
left a comment
There was a problem hiding this comment.
Looks great. I have confirmed that signing still works correctly. I think we're good to go.
|
@anpaz thank you for the review and validating signing. Is there anything else needed from me to merge this PR? |
bettinaheim
left a comment
bettinaheim
left a comment
There was a problem hiding this comment.
@anpaz thank you for the review and validating signing. Is there anything else needed from me to merge this PR?
Looks good. I'll go ahead and make sure it gets merged.
| @@ -1,5 +1,7 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
| // Licensed under the MIT License. | |||
| // <copyright file="LLVMMemoryBufferRefExtensions.cs" company="Ubiquity.NET Contributors"> | |||
There was a problem hiding this comment.
I believe all files in the LLVMSharpExtensions folder were indeed fully authored by us as part of porting. However, I don't see issues with having a partial copyright for all files in the LLVMBindings folder, so if this works better with the stylecop, I believe that should be fine.
There was a problem hiding this comment.
oh, sorry I thought that default header was used in those files by mistake.
It really simplifies things a lot to use the Ubiquty.NET header for all files in the project (so it's great that you fine with it, thanks) because StyleCop does not support different headers in the same project and it looks like, currently, there is no way to suppress it in a single file DotNetAnalyzers/StyleCopAnalyzers#1701. The only alternative I see is disabling the check for the project (then custom stylecop.json for the project also could be removed).
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
3 participants
Follow-up after #931
The main idea was to enforce a custom header in the LlvmBindings project.
The downside of it would be that other settings in both
stylecop.jsonwould need to be in sync with each other.I think it worth it since different headers hard to keep in sync manually.
If any other project would need to add custom
stylecop.jsonit would be enough to just add it into the project folder.After removing warning suppression and fixing trivial warnings the only file with a problematic header was
DelaySign.cssince it had a standard header and included in multiple projects.To solve that I've moved sing properties into
AssemblyCommon.props, this has an additional benefit that now there is no need to include fileDelaySign.csin every project, but the downside is that all project includingAssemblyCommon.propswould use it by default. When we don't need signing in the project it could be removed by defining<SignAssembly>false</SignAssembly>.As additional changes, I've defined some convenience flags in MSBuild to simplify some conditions. Also some small fixes of headers in LlvmBinding.
In the future main improvement would be to replace the direct import of
AssemblyCommon.propswithDirectry.Build.propsand I would like to do it as a follow-up, to keep this PR smaller.