-
Notifications
You must be signed in to change notification settings - Fork 395
Adding script to generate compile commands database #5883
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
Open
guimafelipe
wants to merge
46
commits into
main
Choose a base branch
from
user/felipeda/compiledb2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
9ab36b7
Initial script
guimafelipe 3d8dc83
Downloading ms2cc
guimafelipe 8323d8f
Adding compile commands database to git ignore
guimafelipe f6908a3
Changing name of temporary log files
guimafelipe fc32011
Adding ms2cc to git ignore
guimafelipe 0f23f7f
Adding current build file directory to files included in test projects
guimafelipe f215bdd
Adding 2
guimafelipe 061dbcd
Bumping version of ms2cc
guimafelipe 00f9789
Using specific path for almost all includes in the project
guimafelipe a3befce
Not cleaning intermediate files if building local
guimafelipe e2ed1bb
Fixing include path on everything
guimafelipe 1c0141c
Using both bin logs
guimafelipe 213caaf
Testing eol
guimafelipe eb97073
Testing eol 2
guimafelipe c3cddcb
Revert "Testing eol 2"
guimafelipe 46c9ca6
Revert "Testing eol"
guimafelipe 93df0d8
Testing eol 3
guimafelipe 5d24606
Fixing EOF on test vcxproj files
guimafelipe 4a0ad1d
Fixing missing test files
guimafelipe 0d6b13f
Revert "Fixing missing test files"
guimafelipe 5ecd1b2
Revert "Fixing EOF on test vcxproj files"
guimafelipe 221f460
Revert "Testing eol 3"
guimafelipe 7aa3f44
Revert "Fixing include path on everything"
guimafelipe c3b1f8e
Revert "Using specific path for almost all includes in the project"
guimafelipe b7e9fe9
Revert "Adding 2"
guimafelipe fcb6344
Revert "Adding current build file directory to files included in test…
guimafelipe 649a900
Regex magic
guimafelipe 28c7dd2
Small changes on build all ps1
guimafelipe 472d62d
adding suggestion
guimafelipe 53ea440
Adding CleanIntermediateFiles argument to BuildAll.ps1 script
guimafelipe f63dbf3
Improving binlog file discovering
guimafelipe 37f8e52
Removing temporary files
guimafelipe f2cb8e3
Adding more logging
guimafelipe b196ba3
Merge branch 'main' into user/felipeda/compiledb2
guimafelipe 6da3435
Fixing delete intermediate files
guimafelipe 119b742
Adding update functionality to script
guimafelipe 331fa32
Adding documentation
guimafelipe 04a0b16
Fixing casing and identation
guimafelipe 1a2ab6a
Fixing open braces
guimafelipe 695a614
Adding .hpp
guimafelipe 728d2dd
Adding opt in to download ms2cc or using existing path
guimafelipe d465a4e
Removing ms2cc from gitignore
guimafelipe acfe357
Adding hpp part 2
guimafelipe 028635c
Changing most of the not important logging to verbose
guimafelipe 4cffe83
Adding absolute path fix to precompiled header
guimafelipe 5d136de
Moving context path include to front
guimafelipe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the
$(MSBuildThisFileDirectory)prefix on every header and source?VS doesn't do this when you add a file in Solution explorer (new, or point at existing).
Aren't these files relative to the project (and typically in same dir)?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For developers that work on WinAppSDK, it is common that regardless of the code editor used, there is always a lot of errors that does not help with productivity even if the project builds correctly locally. There is also no consistent Language Server Protocol support (things like "Go to Definition", "See references", etc.).
This PR is from some research and work I put to try to make this all available for WinAppSDK developers.
For this, we can use CLang Driver (https://clangd.llvm.org/), available in most of the editors, to have this support. For clangd to work, it needs a "compile commands database", which is basically a JSON file with all the commands executed by the compiler for building each of the files in the project, with the include folders needed for each of them, so clangd can do all the correct linking between the files.
Unfortunately, MSBuild does not produce a compile commands database naturally, so we need to use an external tool for that. There are some Visual Studio extensions for that, but they not work because our build system uses powershell scripts moving lots of stuff around before even calling msbuild directly.
In my research, I found the https://github.com/freddiehaddad/ms2cc tool, from another microsoft software engineer, that was attempting to solve the same problem in their project. This tool works getting a msbuild log and converting it into a compile commands database, which is what the powershell script I added is doing.
The problem is: if we don't pass
$(MSBuildThisFileDirectory)to CLCompile includes on our project files, the compile command will be something like:cl.exe ... [includess] /Fo="C:\WindowsAppSKD\obj\..." pch.cppThis would not be a problem is there were only one file named pch.cpp in the project. We could search for it in our file tree and know what is the full path. But there are various
pch.cppfiles in WinAppSDK, and there is no way to know only from the compilation path. We could use the object folder for that (which was also being deleted, losing track of some of the include files referenced by the tests), but there is no pattern to follow either.By including it with
$(MSBuildThisFileDirectory), it will have the full path for every.hor.cppfile compiled in the msbuild logs, which will make the linking possible as there will be no ambiguity of which file is being referenced.This way, the text editors now have full access to the LSP beneficies from clangd and there will be no more weird errors everywhere even though everything builds correctly.
WinAppSDK was already using
$(MSBuildThisFileDirectory)in part of the includes, and broadcasted it to the rest of the project and it worked really well for generating the database. I am currently investigating if there is a way to set VS to add it automatically.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is VS is our primary build tool, and adding files to projects is commonly done via VS' Solution Explorer.
I didn't say primary editor. Some use VS, I (like many) rarely use VS' editor (I'm a longstanding TSE fan, others use VScode and other tools). But for build and project + solution management VS is the primary tool.
While command line support is nice, I'm not keen on the friction of requiring
$(MSBuildThisFileDirectory)prefixed on all files in *proj when VS doesn't do that. I don't see us banning VS, and most devs aren't going to know better (or human error will occasionally forget) to edit *.*proj files after making any edits in VS. This is just going to lead to a constant tax where command line builds regularly break as most devs expand their projects using VS, and not this additional manual editing step.(Which, BTW, the VS team says "Do not directly edit *.*proj files." Even though many do, or need to do because the GUI lacks some GUI means to make certain edits.
This will create pains and friction for the team. That's the wrong vector - we want LESS friction for the dev team, not more.
TL;DR I'm sympathetic to letting devs use their preferred tools, but not in this way. Not benefiting some developers by imposing a permanent and recurring tax on all future development. The ROI isn't justified.
Have you talked to the VS team about this? That seems the better answer -- for VS to create/manage a compiler command database and any related changes needed (e.g. $(MSBuildThisFile)foo.cpp). Then more tools are supported but w/o the added dev tax to do non-VS-standard behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also use Visual Studio regularly, which is also a editor. Clangd is also naturally supported in Visual Studio, which is also currently having this same problems as the other editors (showing errors where there is none, not having complete support for C/C++ LSP and so on).
I agree with you that having
$(MSBuildThisFileDirectory)is a big friction and I am still trying to find alternatives to that. For now was the only thing that make it possible for us to correctly index all the files.I didn't talk to them yet. I was expecting that generating a command compile database would be something that already existed by default, but it most of the tools I found related to that are external and didn't work well for our case either. I like the idea of starting a discussion about it with them.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guimafelipe I'm also concerned with having to modify every ClCompile and ClInclude. Note that ClInclude is only an IDE amenity - it has nothing to do with the build. If you're processing the binlog to find the $task cl, you should be able to get the enclosing vcxproj location and infer the source location from that.
For completeness, another approach: cvdump on the pdb to collect the cl.exe command line and all parameters. But my preference would be groveling the binlog for the vcxproj location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DrusTheAxe Thank you for the feedback, I worked on an alternative via powershell scripts for that and all is working fine now.
@Scottj1s Thank you for the tip on looking for the rest of the logs for context I could use to regenerate the full paths!