Added API to manually run CMake - #1483
Conversation
|
Hi Mr. @betamaxbandit , Before making this PR official, could you take a quick look and let me know what you think? FYI, it built failed due to the removal of Thanks. |
db124ca to
6b81f27
Compare
Hi @DangMinhTam382 , |
Allows ISV to manually run CMake without having to build the project. Moved code to separate method and make a convenience protected for the derived to call.
6b81f27 to
f70ef1b
Compare
|
Hi Mr. @betamaxbandit , I have made some modification to allow manual testing for this PR. Many thanks, |
betamaxbandit
left a comment
There was a problem hiding this comment.
LGTM, just a few minor grammar issues.
|
|
||
| ### 9) CMake configuration | ||
|
|
||
| Verifies that API for configuring CMake could work individually. |
There was a problem hiding this comment.
Verifies that the API for configuring CMake can be invoked independently.
There was a problem hiding this comment.
Thanks for the suggestion! I updated the README.md file
| ### 9) CMake configuration | ||
|
|
||
| Verifies that API for configuring CMake could work individually. | ||
| Note, API could only be test in Developer environment with plug-in **org.eclipse.cdt.cmake.example** included. |
There was a problem hiding this comment.
Note: this API can only be tested in a CDT development environment that includes the org.eclipse.cdt.cmake.example plug-in.
There was a problem hiding this comment.
Thanks for the suggestion! I updated the README.md file
| 1. Remove existing "**/build/**" folder. | ||
| 2. Right click project > select "Configure CMake Project". | ||
|
|
||
| Expected: CMake Configuration process start with active launch settings. |
There was a problem hiding this comment.
- Remove any existing
**/build/**folder. - Right-click the project and select Configure CMake Project.
Expected: The CMake configuration process starts using the active launch settings.
There was a problem hiding this comment.
Thanks for the suggestion! I updated the README.md file
| /** | ||
| * @since 2.1 | ||
| */ | ||
| protected IStatus configureCMakeBuildFiles(IProgressMonitor monitor) throws CoreException, IOException { |
There was a problem hiding this comment.
This helper now refreshes and processes compile_commands.json, but build() continues to do the existing compile_commands.json refresh/processing immediately afterwards. So a normal build that needs to run CMake appears to parse compile_commands.json twice.
Could we split this into two helpers, e.g. one method that only runs the CMake configure command and another that refreshes/processes compile_commands.json? Then build() can keep its existing single compile_commands.json processing step, while the public configure-only API can call both helpers.
Something like ...
private IStatus runCMakeConfigure(CommandDescriptorBuilder cmdBuilder, IConsole console,
ConsoleOutputStream infoStream, IProgressMonitor monitor)
throws CoreException, IOException {
// run CMake only
}
private void refreshAndProcessCompileCommandsFile(IConsole console, IProgressMonitor monitor)
throws CoreException {
getCompileCommandsFile().refreshLocal(IResource.DEPTH_ZERO, monitor);
processCompileCommandsFile(console, monitor);
}
There was a problem hiding this comment.
Ahh, you're right there!
It does parse compile_commands.json file twice.
Code updated as your suggestion!
| */ | ||
| protected IStatus configureCMakeBuildFiles(IProgressMonitor monitor) throws CoreException, IOException { | ||
| IProject project = getProject(); | ||
| project.deleteMarkers(ICModelMarker.C_MODEL_PROBLEM_MARKER, false, IResource.DEPTH_INFINITE); |
There was a problem hiding this comment.
I do not think the configure-only API should delete all CDT problem markers. A manual CMake configure does not run the compiler, so clearing previous compiler diagnostics could hide real errors even though no build has replaced them.
The private helper already clears CMake execution markers via deleteCMakeErrorMarkers(getProject()), which seems appropriate. I suggest removing this broader ICModelMarker.C_MODEL_PROBLEM_MARKER deletion from the configure-only API and leaving broad marker cleanup to the full build path only.
There was a problem hiding this comment.
Agreed!
Code updated, no longer remove ICModelMarker.C_MODEL_PROBLEM_MARKER when only configure CMake
| String arg0 = command.getArguments().get(0); | ||
| if (p == null) { | ||
| // process start failed | ||
| String msg = String.format(Messages.CMakeBuildConfiguration_Failure, ""); //$NON-NLS-1$ |
There was a problem hiding this comment.
Since this message is now returned to callers as an IStatus, it would be useful to include the command that failed to start. Could this use arg0 instead of the empty string?
something like ...
String msg = String.format(Messages.CMakeBuildConfiguration_Failure, arg0);
There was a problem hiding this comment.
I had some thought on this one.
Since the condition is checking for process starting error, instead of using arg0, the message is updated to
Failure running cmake: Process failed to start
Error related to arg0 with be check/log in the next condition
if (exitValue != 0) {
// cmake had errors...
String msg = String.format(Messages.CMakeBuildConfiguration_ExitFailure, arg0, exitValue);
addMarker(srcFolder.getProject(), -1, msg, IMarkerGenerator.SEVERITY_ERROR_BUILD, null);
return Status.error(msg);
}
WDT?
|
|
||
| /** | ||
| * @since 2.1 | ||
| */ |
There was a problem hiding this comment.
Can you add a good comment seeing as this is going to be API.
Something like ...
/**
* Runs the CMake configure step for this build configuration without invoking
* the build target. On success, refreshes and processes compile_commands.json
* so scanner information is updated.
* <p>
* This method writes to the CDT build console, deletes stale CMake execution
* markers, may create new CMake execution markers, and updates scanner
* information for this build configuration. Callers should run this from a
* background workspace operation, not directly from the UI thread.
*
* @param monitor progress monitor, or {@code null}
* @return {@link Status#OK_STATUS} if CMake completed successfully; otherwise
* an error status if the CMake process could not be started or exited
* non-zero
* @throws CoreException if workspace refresh, marker handling, or scanner-info
* processing fails
* @throws IOException if console/process I/O fails
* @since 2.1
*/
public IStatus configureCMakeBuildFiles(IProgressMonitor monitor)
throws CoreException, IOException {
There was a problem hiding this comment.
Ahhh, thanks your suggestion.
That's a big help for me.
| addMarker(srcFolder.getProject(), -1, msg, IMarkerGenerator.SEVERITY_ERROR_BUILD, null); | ||
| return null; | ||
| } | ||
| IStatus result = configureCMakeBuildFiles(cmdBuilder, console, infoStream, monitor); |
There was a problem hiding this comment.
This helper now refreshes and processes compile_commands.json, but build() continues to do the existing compile_commands.json refresh/processing immediately afterwards. So a normal build that needs to run CMake appears to parse compile_commands.json twice.
Could we split this into two helpers, e.g. one method that only runs the CMake configure command and another that refreshes/processes compile_commands.json? Then build() can keep its existing single compile_commands.json processing step, while the public configure-only API can call both helpers.
Something like ...
private IStatus runCMakeConfigure(CommandDescriptorBuilder cmdBuilder, IConsole console,
ConsoleOutputStream infoStream, IProgressMonitor monitor)
throws CoreException, IOException {
// run CMake only
}
private void refreshAndProcessCompileCommandsFile(IConsole console, IProgressMonitor monitor)
throws CoreException {
getCompileCommandsFile().refreshLocal(IResource.DEPTH_ZERO, monitor);
processCompileCommandsFile(console, monitor);
}
There was a problem hiding this comment.
Ahh, you're right there!
It does parse compile_commands.json file twice.
Code updated as your suggestion!
| ILog.of(ConfigureExtendedCMakeProjectHandler.class) | ||
| .error("Failed to configure for extended CMake Project", e); //$NON-NLS-1$ | ||
| } | ||
| return null; |
There was a problem hiding this comment.
runInWorkspace should return an IStatus. Returning null after logging the exception is risky and means the Jobs framework does not receive a proper failure status.
Could this return the CoreException status, or an error status for IOException?
Something like ...
} catch (CoreException e) {
return e.getStatus();
} catch (IOException e) {
ILog.of(ConfigureExtendedCMakeProjectHandler.class)
.error("Failed to configure for extended CMake Project", e); //$NON-NLS-1$
return Status.error("Failed to configure for extended CMake Project", e); //$NON-NLS-1$
}
There was a problem hiding this comment.
Ahh, thanks for pointing this out and the suggestion as well.
I believe the WorkspaceJob does log the error if an error status is return.
So, I grouped them together and return a simple
Status.error(message, exception)
WDT?
| return null; | ||
| } | ||
| }; | ||
| job.schedule(); |
There was a problem hiding this comment.
This job modifies project state: markers, resource refresh, and scanner information. Should it set a scheduling rule before scheduling, probably the project, to avoid running concurrently with other workspace operations on the same project?
Suggestion ...
job.setRule(project);
job.schedule();
There was a problem hiding this comment.
I'm with you on this one.
I'm not quite sure if there's another WS job running during CMake configuration process, so I'll play it safe here and set rule to it as suggested.
| point="org.eclipse.ui.commands"> | ||
| <command | ||
| id="org.eclipse.cdt.cmake.example.configureCMakeProject" | ||
| name="Configure Extended CMake Project"> |
There was a problem hiding this comment.
The menu label says "Configure CMake Project" but the command name says "Configure Extended CMake Project". Could these be made consistent? Unless "Extended" has a specific meaning here, I suggest using "Configure CMake Project" in both places.
There was a problem hiding this comment.
Ah, yes, the initial intent was to allow CMake configuration only for Extended CMake projects.
Later, this was changed to support all CMake projects, and these spots were somehow missed.
Updated the UI and class names to align with the current behavior.
betamaxbandit
left a comment
There was a problem hiding this comment.
Hi,
this is looking really good.
There's a few minor issues that should be resolved please.
a8aa06f to
6314dbd
Compare
DangMinhTam382
left a comment
There was a problem hiding this comment.
Hi Mr. @betamaxbandit ,
I fixed your comments and this should be ready to be reviewed again
Many thanks,
Tam
|
|
||
| /** | ||
| * @since 2.1 | ||
| */ |
There was a problem hiding this comment.
Ahhh, thanks your suggestion.
That's a big help for me.
| addMarker(srcFolder.getProject(), -1, msg, IMarkerGenerator.SEVERITY_ERROR_BUILD, null); | ||
| return null; | ||
| } | ||
| IStatus result = configureCMakeBuildFiles(cmdBuilder, console, infoStream, monitor); |
There was a problem hiding this comment.
Ahh, you're right there!
It does parse compile_commands.json file twice.
Code updated as your suggestion!
| return null; | ||
| } | ||
| }; | ||
| job.schedule(); |
There was a problem hiding this comment.
I'm with you on this one.
I'm not quite sure if there's another WS job running during CMake configuration process, so I'll play it safe here and set rule to it as suggested.
|
|
||
| ### 9) CMake configuration | ||
|
|
||
| Verifies that API for configuring CMake could work individually. |
There was a problem hiding this comment.
Thanks for the suggestion! I updated the README.md file
| 1. Remove existing "**/build/**" folder. | ||
| 2. Right click project > select "Configure CMake Project". | ||
|
|
||
| Expected: CMake Configuration process start with active launch settings. |
There was a problem hiding this comment.
Thanks for the suggestion! I updated the README.md file
| */ | ||
| protected IStatus configureCMakeBuildFiles(IProgressMonitor monitor) throws CoreException, IOException { | ||
| IProject project = getProject(); | ||
| project.deleteMarkers(ICModelMarker.C_MODEL_PROBLEM_MARKER, false, IResource.DEPTH_INFINITE); |
There was a problem hiding this comment.
Agreed!
Code updated, no longer remove ICModelMarker.C_MODEL_PROBLEM_MARKER when only configure CMake
| String arg0 = command.getArguments().get(0); | ||
| if (p == null) { | ||
| // process start failed | ||
| String msg = String.format(Messages.CMakeBuildConfiguration_Failure, ""); //$NON-NLS-1$ |
There was a problem hiding this comment.
I had some thought on this one.
Since the condition is checking for process starting error, instead of using arg0, the message is updated to
Failure running cmake: Process failed to start
Error related to arg0 with be check/log in the next condition
if (exitValue != 0) {
// cmake had errors...
String msg = String.format(Messages.CMakeBuildConfiguration_ExitFailure, arg0, exitValue);
addMarker(srcFolder.getProject(), -1, msg, IMarkerGenerator.SEVERITY_ERROR_BUILD, null);
return Status.error(msg);
}
WDT?
| ILog.of(ConfigureExtendedCMakeProjectHandler.class) | ||
| .error("Failed to configure for extended CMake Project", e); //$NON-NLS-1$ | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Ahh, thanks for pointing this out and the suggestion as well.
I believe the WorkspaceJob does log the error if an error status is return.
So, I grouped them together and return a simple
Status.error(message, exception)
WDT?
| point="org.eclipse.ui.commands"> | ||
| <command | ||
| id="org.eclipse.cdt.cmake.example.configureCMakeProject" | ||
| name="Configure Extended CMake Project"> |
There was a problem hiding this comment.
Ah, yes, the initial intent was to allow CMake configuration only for Extended CMake projects.
Later, this was changed to support all CMake projects, and these spots were somehow missed.
Updated the UI and class names to align with the current behavior.
| /** | ||
| * @since 2.1 | ||
| */ | ||
| protected IStatus configureCMakeBuildFiles(IProgressMonitor monitor) throws CoreException, IOException { |
There was a problem hiding this comment.
Ahh, you're right there!
It does parse compile_commands.json file twice.
Code updated as your suggestion!
Allows ISV to manually run CMake without having to build the project.
Moved code to separate method and make a convenience protected for the derived to call.