-
Notifications
You must be signed in to change notification settings - Fork 484
Add new files directly to CMakeLists.txt (#2132) #4454
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
base: main
Are you sure you want to change the base?
Conversation
@gvcallen I would love to get any or all of your feedback on this, since you were involved in one way or another in the original discussions on #2132. Thanks! |
a769247
to
52e78c4
Compare
There's a little corner case in this code. The issue is with figuring out how far to indent the new line with the new source file on it. I don't think the VSCode API gives access to enough information to actually do this perfectly every time without unpleasant UI disruptions. Every time a new file is added to a source list in a CMake list file, it is preceded by a newline followed by an indent: target_sources(my_target
PUBLIC
include/i.h
+ include/foo.h
INTERFACE
FILE_SET other_headers In order to do this perfectly, we would need to know exactly what vscode thinks The only way to get the per-editor overrides or guessed values with the vscode API is through a So I guess my question is -- how hard do we want to work to get a "correct" indentation, and what kinds of UI wonkiness are we willing to put up with in order to do it? There's a simple way to do a pretty good job most of the time, which is to just use the indentation of the previous line. This works unless the previous line is the first line of a command invocation, in which case you probably want to add an additional indentation level. At time of writing, this PR is using the |
5cd0e34
to
5586db5
Compare
I did some playing with Visual Studio, to see how its functionality compares to what I've implemented in this PR. It looks like when it doesn't feel confident modifying CMakeLists.txt automatically, it will present its equivalent of a Refactor Preview to allow the user to select which modifications to apply. I made a quick toy version of this PR that combines all possible list file edits into a single Pros
Cons
LimitationsUnder many circumstances, Visual Studio will initialize its edit selection dialog with every item checked. That isn't possible with the VSCode WorkspaceEdit API -- if every insert has Other differencesVisual Studio is also less aware in some other respects than this PR is.
I wouldn't want to take those capabilities out of this PR, though -- I think they're unobtrusive "nice to haves". Final thoughtsI don't really like using the Refactor Preview for this, personally. It's a situation that only comes up with more complicated CMake configurations, and in those cases I think the extra hand-holding and context that's available in my QuickInput-based implementation makes it worth deviating from a Visual Studio-style implementation. It's an easy enough change to make to this PR, though, so if the maintainers prefer it, I can turn my toy implementation into a real one. |
This parser accepts the grammar described in [cmake-language(7)](https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#syntax). It is somewhat more lenient than that parser, in that it permits comments in places that the official grammar would reject them.
Adds two new commands that scan the Code Model and CMakeLists.txt for command invocations that add source files to targets or certain variables. * cmake.addFileToCMakeLists: Present the user with options for which command invocation to modify, and then edit that command invocation to add a new source file to its arguments. * cmake.removeFileFromCMakeLists: Edit any command invocations that include a source file, removing it from their argument lists. These commands can optionally be triggered when a source file is created or deleted. Quick Pick items for targets and invocations are carefully sorted to make the first option very likely to be the desired one, and settings exist to automatically choose the best option rather than asking.
`window.showTextDocument()` allows access to more accurate indentation settings, but it creates jarring flashes in the UI, and for the corner cases where it makes a difference, I don't think it's worth it.
5586db5
to
6d14247
Compare
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.
Pull Request Overview
This PR introduces commands and underlying support for automatically appending or removing source files in CMakeLists, along with new configuration options and a custom CMake parser.
- Registers two new commands (
addFileToCMakeLists
,removeFileFromCMakeLists
) and hooks them into the ExtensionManager - Extends the File API data model to include
backtraceGraph
andsourceFileExtensions
- Adds a standalone
CMakeParser
for parsing command invocations and a suite of new settings inpackage.json
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/extension.ts | Imports and wires up the new CMakeListsModifier , registers commands, and updates code model callbacks |
src/drivers/codeModel.ts | Adds backtraceGraph and sourceFileExtensions to the CodeModel types and defines shared interfaces |
src/drivers/cmakeFileApi.ts | Populates backtraceGraph and sourceFileExtensions from the File API |
src/cmakeParser.ts | Implements a new CMake command‐invocation parser (CMakeParser ) |
package.json | Adds the two new commands to contributes.commands and defines a new cmake.modifyLists settings section |
Comments suppressed due to low confidence (1)
src/cmakeParser.ts:1
- The new CMakeParser has no accompanying unit tests. Consider adding tests for parsing different invocation styles, comments, and nested parentheses.
import * as vscode from "vscode";
@@ -849,6 +856,16 @@ export class ExtensionManager implements vscode.Disposable { | |||
} | |||
const folder: vscode.WorkspaceFolder = cmakeProject.workspaceFolder; | |||
this.projectOutline.updateCodeModel(cmakeProject, cmakeProject.codeModelContent); | |||
rollbar.invokeAsync('Update code model for automatic list file modifier', {}, async () => { |
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.
Updating the code model for the list file modifier on every invocation may introduce overhead. Consider debouncing these updates or checking for relevant changes before running.
Copilot uses AI. Check for mistakes.
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.
@gcampbell-msft is this a legit suggestion from Copilot? The much longer invokeAsync block just below it, "Update code model for cpptools", doesn't do any kind of debouncing.
This is a bit of the PR that I'm not totally confident in, so I would like feedback on it, but I'm not sure about Copilot's take here.
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.
@malsyned I'm not sure either, I have it on my backlog to review this PR, but in full transparency it may not be until next week or the week after when I get to it.
Thank you for your patience.
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.
No worries! This was a big PR to just drop on you out of nowhere, I appreciate you considering it. By the time I realized how big the patch was going to be, it was already way bigger than I had imagined.
It's far enough along that I don't want to take it any further without some feedback from a maintainer about whether it's headed in the right direction. It's not done yet, though, there's plenty of polish it would need before being ready for merging. Should I convert it to a draft?
This change addresses item #2132
This changes visible behavior
The following changes are proposed:
Add two new commands that edit CMake List files to append or delete source file names from command invocations.
cmake.addFileToCMakeLists
Present the user with options for which command invocation to modify, and then edit that command invocation to add a new source file to its arguments.
cmake.removeFileFromCMakeLists
Edit any command invocations that include a source file, removing it from their argument lists.
These commands can optionally be triggered when a source file is created or deleted.
Quick Pick items for targets and invocations are carefully sorted to make the first option very likely to be the desired one, and settings exist to automatically choose the best option rather than asking.
Add options for controlling the behavior of these commands.
Optionally, present proposed edits to the user to Apply or Discard.
The purpose of this change
Other Notes/Information
Phew. This one wound up being a lot bigger of a bite than I expected when I decided to try to tackle it. Apologies for the surprise 1.5 kloc pull request. I'm assuming/hoping that it's work you're interested in, since it's in response to a 3-year-old feature ticket that had a lot of momentum and maintainer buy-in when it was first introduced.
It's still a draft and needs some polishing before it's merge-ready, but I don't want to let it get any bigger before getting some feedback from the maintainers about whether I'm on the right track. I'd consider it just about feature-complete though, and ready for testing.
Here's an explanation of how this feature works. Consider this a rough draft of the user documentation.
Terminology
I have tried to use this terminology consistently throughout the implementation and documentation.
CMakeLists.txt
files and other*.cmake
files used viainclude()
or similar mechanisms.add_subdirectory
on the directory that contains it. (This distinction matters because the Backtrace Graph code path can find command invocations in any CMake List file that contributes to the build system, but the parser-based legacy fallback path, and the variable invocation search, only take into account files namedCMakeLists.txt
.)add_executable
,add_library
, oradd_custom_target
.target_sources
,add_executable
,add_library
, or a user-defined function or macro that in turn invokes one of those commands.set()
,list(APPEND)
,list(PREPEND)
, orlist(INSERT)
.target_sources
, can contain more than one source list with different scopes or file set types.Selecting targets, command invocations, and source lists
If only one option is identified at any of these steps, it is used without prompting the user, to reduce the amount of user interaction to only what is necessary. Additionally, every step has a corresponding
auto
setting to choose the highest-sorted option automatically instead of prompting the user.Targets
Identifying
project.currentBuildType()
are considered.sourceDirectory
contains the new file are considered.cmake.modifyLists.targetSelection
isaskNearestSourceDir
, only targets defined in the closest CMake List file are considered. Otherwise, all parent directories up to the project root are considered.Sorting
Target Source Command Invocations
Identifying
Command invocations are identified using different methods depending on whether the file API, which provides a
backtraceGraph
for each target, is available.From Backtrace Graph
cmake.modifyLists.targetSourceCommands
. The backtraces for those frames are then walked to find the outermost line of code that caused that command to be invoked.Fallback: parse
CMakeLists.txt
filesCMakeLists.txt
in the same directory as the new file and every parent directory up to the project root are considered.cmake.modifyLists.targetCommandInvocationSelection
isaskFirstParentDir
, only the first one encountered will be examined.cmake.modifyLists.targetSourceCommands
are identified.CMakeLists.txt
. Variable substitution is not performed on their names. (This is a disadvantage compared to the Backtrace Graph method, which is operating on data produced by CMake after all variable substitution has been performed and all generator expressions have been evaluated.)Sorting
CMakeLists.txt
are sorted before other list files.if()
,for()
, orwhile()
blocks are less likely to be "default dumping grounds" for source listscmake.modifyLists.targetSourceCommands
, then alphabetically by file name, then by line number.Source Lists
Every target source command invocation produces at least one source list for consideration, but some can produce more than one, such as:
target_sources
command with multiple scopes (e.g. aPUBLIC HEADERS
file set and aPRIVATE
-scoped set of.c
/.cxx
files).parse_arguments
to have more than one multi-value source list.Identifying
PUBLIC
,PRIVATE
, orINTERFACE
, then its argument list is parsed as for atarget_sources
command (regardless of the actual command name), and any identified scopes or file sets are produced.if
cmake.modifyLists.sourceListKeywords
has any entries, then keyword arguments are only considered to introduce source lists if they match an element of that array. Otherwise, any argument matching/^[A-Z_]+$/
is assumed to introduce a multi-value source list.Sorting
FILE_SET
lists are only shown for header files and C++ module files. When they are, they appear before non-FILE_SET
scopes.HEADERS
,CXX_MODULES
).PRIVATE
first, thenINTERFACE
, thenPUBLIC
.PUBLIC
first, thenINTERFACE
, thenPRIVATE
.parse_arguments
, are sorted by the order their keywords appear incmake.modifyLists.sourceListKeywords
.Variable
set()
andlist()
invocationsIf
cmake.modifyLists.variableSelection
isnever
(the default), then variable command invocations will not be modified by any command, and the user will never be offered to select variable command invocations to modify.Conversely, if that setting has any other value, any eligible variable command invocations are found in the tree above a new source file, and the user cancels instead of selecting one, no other command invocation options will be offered.
Identifying
CMakeLists.txt
in the same directory as the new file and every parent directory up to the project root are considered.cmake.modifyLists.variableSelection
isaskFirstParentDir
, only the first one encountered will be examined.set()
,list(APPEND)
,list(PREPEND)
, andlist(INSERT)
commands are identified.set()
invocations whose<variable>
arguments, andlist()
invocations whose<list>
arguments, match against the patterns incmake.modifyLists.sourceVariables
are considered. Arguments are compared exactly as they appear in theCMakeLists.txt
. Variable substitution is not performed on their names.Sorting
cmake.modifyLists.sourceVariables
.Remaining Work
docs/
.targetSourceCommands
.null
Code Model.CMakeLists.txt
when the first source file is created in an empty directory.target_sources
.Questions
ConfigReader
instead of reading directly from aWorkSpaceConfiguration
? If so, how is that meant to be used?backtraceGraph
is exported from the File API, is there more I should be doing to post-process it?project.sourceDir
the right thing to use for the "top-level source directory" as defined by the File API?isGenerated
property from the File API'scmakeFiles
object. What's the best way to make that available in the code model?