-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Huge refactor of project files to make compilation security settings easier #3275
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 5444473.
Would disabling RTTI (Run-Time Type Information) in some of the projects decrease the build size? |
Not sure how big of the impact when this is brought to internal build (Added a Directory.Build.props file that is automatically included by MSBuild). So I guess we would merge this after 1.5 release candidate. |
It definitely is something to do to reduce final build size, but I have no idea how much it reduces intermediates size. We can probably do it, but it will take a bit of work to remove the |
Also note that there is already such a file in the internal build and so we would need to be careful about how we get the properties since these projects need to build:
Not sure what the lookup algorithm is for the props file, but I would guess that it isn't directly friendly to the tangled web we've woven. Also, the internal ADO builds use a different props file from the local builds... I suppose what I'm saying is, maybe the props file that these projects use should be explicitly referenced as a project file relative path rather than using the implicit props file inclusion strategy of MSBuild. |
Oh, yeah. I was already thinking of that but forgot to write it in the description
The algorithm basically is "walk up until you find one", but it is possible to import one in upper levels to chain them. I'd need to figure out how to choose which one to use, though. Or change it to what you suggest. I'm also planning on doing this same thing on the internal project, btw. |
Context
I have a bit of a story for this huge PR...
I am working on making sure we have all the right static analysis tools in the pipeline and resolve all warnings. Some warnings were related to using the right security settings for the compiler, like Spectre mitigations, or Control Flow Guard. It was bothering me how each time I added one to a project, VS was adding about eight lines like
<SDLCheck Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">true</SDLCheck>
So I thought "I'll manually edit the
.vcproj
files to remove the conditions so I can have just one instead of eight". Then I realized I was putting the same properties everywhere, and researched how to have common properties in a single file and then I got a bit carried away.Changes
Added a
Directory.Build.props
file that is automatically included by MSBuild early in the process of reading the project files for all the files in the directory. This file includes all the common properties in all the C++ projects, but since it also would apply to the external projects and C# projects, it first detects what project it is in and does something different in each case.Removed everything that was duplicated in the project files. Also removed unused
PropertySheet.props
files. I'm pretty proud of this diff stat:100 files changed, 502 insertions(+), 3261 deletions(-)
For static analysis:
Enabling code analysis and setting warnings at high level across the board created a lot of warnings/errors that I fixed:
Notes
I'm opening this as draft because I haven't figured out some pipeline issues. The changes I did increase the build output size dramatically, from around 8GB to 11GB, which brings us over the free space available in the agent and causes build to fail.
I still have some code to remove. In the pipelines definition I have manually enabled CodeQL for my branch and added some build tasks to investigate the space issues, that needs to be removed. I also added a condition to the properties file to try and disable the security settings to see if that makes a difference, that also needs to be removed
Microsoft Reviewers: Open in CodeFlow