Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@AndreyTretyak
Copy link
Contributor

@AndreyTretyak AndreyTretyak commented Apr 17, 2021

Follow-up after #931

This change replaces manual import of AssemblyCommon.props file with automatic (using Directory.Build.props), so it would be automatically included in every SDK-based project in the src folder, so it's harder to forget it.

Directory.Build.props also allows defining properties for both SDK and non-SDK projects, so I would suggest using it in the future instead of AssemblyCommon.cs.v.template (it might require some changes in build the logic that provides #ASSEMBLY_VERSION#).

An additional change in this PR is a move of InternalsVisibleTo attributes into project files directly. This allows using same approach for both C# and F# projects and replaces two helper files with public key to have a single place for it.

Some of the projects (LanguageServer and Compiler) have a lot of StyleCop warnings that would fail build, so I've disabled StyleCop for them (same as it was before), to avoid making PR too big. Also, StyleCop disabled for Parser project since it looks like code in it autogenerated.

This change treats warnings as errors, so I had to silence some warnings for Parser project, since the code in it autogenerated.

@bamarsha
Copy link
Contributor

bamarsha commented Apr 19, 2021

Some of the projects (LanguageServer and Compiler) have a lot of StyleCop warnings that would fail build, so I've disabled StyleCop for them (same as it was before)

Hmm, I thought the LanguageServer and Compiler projects already had StyleCop enabled in main (see #513), so there shouldn't be any warnings. Are they using the same StyleCop settings as the other projects?

@AndreyTretyak
Copy link
Contributor Author

Some of the projects (LanguageServer and Compiler) have a lot of StyleCop warnings that would fail build, so I've disabled StyleCop for them (same as it was before)

Hmm, I thought the LanguageServer and Compiler projects already had StyleCop enabled in main (see #513), so there shouldn't be any warnings. Are they using the same StyleCop settings as the other projects?

Thank you for noticing it, and sorry for not checking it more thoroughly. Not sure why I was getting warnings about it before (possible some intermediate result got cached or due to other warnings I had) after enabling it back now I don't see those warnings.
I've pushed a change to enable it, let's see how PR build would go.
Thanks again for noticing it!

@AndreyTretyak
Copy link
Contributor Author

@SamarSha I've removed unneeded StyleCop suppression. Could you please have a look once more?
Also, it would be great if you could validate that build that sings your dlls also works (since I've changed InternalVisibleTo attribute).

@bamarsha bamarsha self-requested a review April 23, 2021 17:31
Copy link
Contributor

@bamarsha bamarsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AndreyTretyak, really sorry for not reviewing this earlier. It looks great, I just had a couple questions. Thanks!

@AndreyTretyak
Copy link
Contributor Author

Hey @AndreyTretyak Andrey Tretyak FTE, really sorry for not reviewing this earlier. It looks great, I just had a couple questions. Thanks!

Not a problem at all, I totally understand that this work goes in addition to your regular workload planning. Thank you for the review.

@bamarsha
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@bamarsha bamarsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I am happy about reducing some of the build system boilerplate. :) The signed build also passed. Thanks for the contribution!

@bamarsha bamarsha merged commit 5a90bed into microsoft:main May 11, 2021
@AndreyTretyak AndreyTretyak deleted the andreyt/simplify-common-props branch May 12, 2021 00:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants