IEP-1247 Run CMake initial build for newly created project(idf.py reconfigure) #1051
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the functionality of the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (2)
181-239: LGTM! The changes improve the user experience and automate the reconfiguration process.The method
runCommandIdfPyInIdfEnvis well-structured and handles the execution of theidf.py reconfigurecommand effectively. It captures the command output and displays it in the console, providing real-time feedback to the user.A few suggestions for further improvement:
- Consider extracting the timeout value (
waitCount) into a constant for better readability and maintainability.- Handle the case when the IDF environment variables are not set correctly, as the method relies on them for constructing the command.
266-287: LGTM! The method enhances cross-platform compatibility by determining the correct Python executable path.The
pythonVirtualExecutablePathmethod is well-structured and handles the construction of the Python executable path based on the operating system. It enhances cross-platform compatibility by appending the appropriate directory and filename.One suggestion for further improvement:
- Consider adding error handling or a fallback mechanism in case the
IDF_PYTHON_ENV_PATHenvironment variable is not set, as the method relies on it to retrieve the Python virtual environment path.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (5 hunks)
Additional comments not posted (1)
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (1)
241-264: LGTM! The method ensures that a console with the given name exists and is visible to the user.The
openConsolemethod is well-implemented and handles the creation and activation of the console effectively. It avoids creating duplicate consoles by searching for an existing one before creating a new one.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ConsoleManager.java (2)
12-18: Add Javadoc to explain utility class pattern.Consider adding class-level documentation to explain that this is a utility class and why instantiation is prevented.
+/** + * Utility class for managing Eclipse console instances. + * This class should not be instantiated. + */ public class ConsoleManager { private ConsoleManager() { + // Prevents instantiation of utility class }
31-41: Consider safer type casting and modern Java practices.The method could benefit from these improvements:
- Add method documentation
- Add type checking before casting
- Consider using Optional for better null handling
+/** + * Finds an existing MessageConsole with the specified name. + * + * @param consoleName the name of the console to find + * @return Optional<MessageConsole> containing the found console, or empty if not found + */ -private static MessageConsole findConsole(String consoleName) +private static Optional<MessageConsole> findConsole(String consoleName) { for (IConsole existing : ConsolePlugin.getDefault().getConsoleManager().getConsoles()) { - if (consoleName.equals(existing.getName())) + if (existing instanceof MessageConsole && consoleName.equals(existing.getName())) { - return (MessageConsole) existing; + return Optional.of((MessageConsole) existing); } } - return null; + return Optional.empty(); }Then update the getConsole method accordingly:
public static MessageConsole getConsole(String consoleName) { - MessageConsole console = findConsole(consoleName); - if (console == null) + return findConsole(consoleName).orElseGet(() -> { - console = new MessageConsole(consoleName, null); - ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console }); - } - ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console); - return console; + MessageConsole newConsole = new MessageConsole(consoleName, null); + ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { newConsole }); + ConsolePlugin.getDefault().getConsoleManager().showConsoleView(newConsole); + return newConsole; + }); }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/messages.properties (1)
30-30: Consider enhancing the reconfigure message for clarity.While the message is functional, it could be more informative about the purpose of reconfiguration.
Consider this alternative:
-IdfReconfigureJobName=Running idf.py reconfigure command... +IdfReconfigureJobName=Running idf.py reconfigure to initialize CMake build...bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IdfCommandExecutor.java (3)
27-37: Add class documentation and null validation.Consider adding:
- JavaDoc documentation for the class explaining its purpose and responsibilities
- Documentation for the fields explaining their roles
- Null validation in the constructor
+/** + * Executes IDF (IoT Development Framework) commands within a project context. + * This executor handles the reconfiguration of IDF projects using the specified target platform. + */ public class IdfCommandExecutor { + /** The target platform for IDF configuration */ private final String target; + /** Console for command output */ private final MessageConsole console; public IdfCommandExecutor(String target, MessageConsole console) { + if (target == null || console == null) { + throw new IllegalArgumentException("Target and console must not be null"); + } this.target = target; this.console = console; }
39-43: Add method documentation.Add JavaDoc to explain the method's purpose, parameters, and return values.
+/** + * Executes the reconfigure command for the given project. + * Activates the console and runs the IDF reconfigure command. + * + * @param project the project to reconfigure + * @return IStatus indicating success or failure of the operation + */ public IStatus executeReconfigure(IProject project)
1-93: Consider implementing a more robust command execution framework.Given that this class is part of a larger IDF command execution system, consider:
- Creating an abstract base class or interface for command execution to standardize error handling, timeout management, and output processing across different commands
- Implementing a command pattern to encapsulate different IDF commands
- Adding a command result object that includes:
- Exit code
- Output/error streams
- Execution duration
- Resource usage
- Implementing proper cleanup of processes and resources
- Adding support for command cancellation
This would make the system more maintainable and extensible for future IDF commands.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java (1)
Line range hint
1-150: Consider enhancing error handling and documentation for CMake integration.Since this class is now part of the CMake initial build functionality:
- Consider adding CMake-specific error detection and handling in
processData- Document the class's role in the CMake build process
- Consider adding debug logging for better troubleshooting of CMake issues
Would you like help with implementing any of these suggestions?
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (2)
68-69: Remove unused fieldconsole.The
consolefield is declared but never used throughout the class. Consider removing it to maintain code cleanliness.private IProject project; -private MessageConsole console;
147-166: Consider moving the reconfigure job to a separate class.The job implementation would be better placed in a separate class to improve maintainability and separation of concerns. This would also make it easier to test and reuse the functionality.
Consider creating a new class like
IDFProjectReconfigureJobthat extendsJoband encapsulates all the reconfiguration logic. This would make the wizard code cleaner and more focused on its primary responsibility.bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (2)
397-400: Improve working directory setup readability.The indentation is inconsistent with the rest of the file. Also, consider adding a comment to explain the fallback behavior.
Apply this diff to improve readability:
String terminalWrkDir = getWorkingDir(); -if (StringUtil.isEmpty(terminalWrkDir)) { - terminalWrkDir = workingDir; -} +// Fall back to the provided working directory if getWorkingDir() returns empty +if (StringUtil.isEmpty(terminalWrkDir)) { + terminalWrkDir = workingDir; +}
Line range hint
437-449: Add null checks and improve method documentation.The method needs several improvements:
- Add null checks to prevent NPE in the resource handling chain.
- Add method documentation to explain the logic and return value.
- Consider logging when falling back to IDF path.
Apply this diff to fix the issues:
+/** + * Returns the working directory for the terminal. + * First tries to get the directory from the selected resource's project. + * Falls back to IDF path if no resource is selected or if resource handling is unavailable. + * + * @return The working directory path as a string, never null + */ protected String getWorkingDir() { Bundle bundle = Platform.getBundle("org.eclipse.core.resources"); //$NON-NLS-1$ if (bundle != null && bundle.getState() != Bundle.UNINSTALLED && bundle.getState() != Bundle.STOPPING) { // if we have a IResource selection use the location for working directory IResource resource = EclipseUtil.getSelectionResource(); - if (resource instanceof IResource) { + if (resource != null && resource.getProject() != null && resource.getProject().getLocation() != null) { String dir = ((IResource) resource).getProject().getLocation().toString(); return dir; } } + // Fall back to IDF path when resource selection is not available return IDFUtil.getIDFPath(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java(2 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ConsoleManager.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IdfCommandExecutor.java(1 hunks)bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/Messages.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java(5 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/messages.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/Messages.java
🔇 Additional comments (11)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ConsoleManager.java (2)
1-11: LGTM! File structure and imports are well-organized.
The copyright header, package declaration, and imports are properly structured and contain only the necessary dependencies.
12-42: Verify thread safety for concurrent console creation.
Since this utility class is used by NewIDFProjectWizard, multiple wizard instances could potentially create race conditions when creating/accessing consoles concurrently.
✅ Verification successful
Based on the search results, I can now provide a final response:
ConsoleManager is thread-safe for console creation and access
The ConsoleManager implementation is inherently thread-safe because:
- It delegates to Eclipse's ConsolePlugin which handles thread synchronization internally
- The only usage found shows ConsoleManager.getConsole() being called from the UI thread in NewIDFProjectWizard
- The findConsole() method performs a read-only operation on the console list
- Console creation is handled atomically through ConsolePlugin's addConsoles() method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential concurrent usage of ConsoleManager
# Test: Search for concurrent access patterns
rg -A 5 "ConsoleManager.getConsole"
# Test: Check if ConsoleManager is used in any async/parallel execution contexts
ast-grep --pattern 'new Thread($$$)'
ast-grep --pattern 'CompletableFuture<$_>($$$)'
ast-grep --pattern 'Job($$$)'
Length of output: 20291
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/messages.properties (1)
29-29: LGTM!
The project name label text is clear and consistent with other labels in the file.
bundles/com.espressif.idf.core/META-INF/MANIFEST.MF (2)
66-67: LGTM! The package import addition is well-structured.
The addition of org.eclipse.ui.console to the Import-Package section is correctly formatted and aligns with the introduction of console management functionality.
67-67: Verify the ConsoleManager implementation.
Let's verify that the ConsoleManager class exists and properly utilizes this newly imported package.
✅ Verification successful
Let me generate another script to examine the ConsoleManager implementation more closely since we found its location.
ConsoleManager implementation properly utilizes org.eclipse.ui.console package
The ConsoleManager class is well-implemented and correctly uses the org.eclipse.ui.console package:
- It properly imports required console classes (ConsolePlugin, IConsole, MessageConsole)
- Implements standard console management patterns with findConsole() and getConsole() methods
- Correctly uses ConsolePlugin for console registration and management
- Has appropriate usage across multiple UI components as shown by the references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of ConsoleManager
# Expected: Find ConsoleManager class with imports from org.eclipse.ui.console
# Search for ConsoleManager class definition
echo "Searching for ConsoleManager class definition:"
ast-grep --pattern 'class ConsoleManager {
$$$
}'
# Search for console package usage
echo -e "\nVerifying org.eclipse.ui.console package usage:"
rg "import org\.eclipse\.ui\.console\." -A 3
Length of output: 14295
Script:
#!/bin/bash
# Let's examine the ConsoleManager implementation
echo "ConsoleManager implementation:"
cat bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ConsoleManager.java
echo -e "\nSearching for ConsoleManager usage:"
rg "ConsoleManager" -l
Length of output: 1904
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IdfCommandExecutor.java (2)
1-26: LGTM! File structure and imports are well-organized.
72-92:
Improve process execution robustness.
The current process execution implementation has several potential issues:
- No timeout handling for long-running processes
- Error stream is not consumed, which might cause process to hang
- Process exit code is not checked
Consider implementing these improvements:
private IStatus runProcess(List<String> arguments, Map<String, String> environment,
ProcessBuilderFactory processRunner, IProject project, MessageConsoleStream messageConsoleStream)
{
StringBuilder output = new StringBuilder();
- try (BufferedReader reader = new BufferedReader(new InputStreamReader(
- processRunner.run(arguments, project.getLocation(), environment).getInputStream())))
+ try
{
- String line;
- while ((line = reader.readLine()) != null)
- {
- output.append(line).append(System.lineSeparator());
- messageConsoleStream.println(line);
+ Process process = processRunner.run(arguments, project.getLocation(), environment);
+
+ // Handle both output and error streams
+ Thread outputThread = startStreamReader(process.getInputStream(), output, messageConsoleStream);
+ Thread errorThread = startStreamReader(process.getErrorStream(), output, messageConsoleStream);
+
+ // Wait for process with timeout
+ if (!process.waitFor(5, TimeUnit.MINUTES)) {
+ process.destroyForcibly();
+ return IDFCorePlugin.errorStatus("Process timed out after 5 minutes", null);
}
- return new Status(IStatus.OK, IDFCorePlugin.PLUGIN_ID, output.toString());
+
+ // Wait for stream readers to complete
+ outputThread.join();
+ errorThread.join();
+
+ int exitCode = process.exitValue();
+ if (exitCode != 0) {
+ return IDFCorePlugin.errorStatus("Process failed with exit code: " + exitCode, null);
+ }
+
+ return new Status(IStatus.OK, IDFCorePlugin.PLUGIN_ID, output.toString());
}
catch (Exception e)
{
Logger.log(e);
return IDFCorePlugin.errorStatus(e.getMessage(), e);
}
}
+private Thread startStreamReader(InputStream input, StringBuilder output, MessageConsoleStream console) {
+ Thread thread = new Thread(() -> {
+ try (BufferedReader reader = new BufferedReader(new InputStreamReader(input))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ synchronized (output) {
+ output.append(line).append(System.lineSeparator());
+ }
+ console.println(line);
+ }
+ } catch (IOException e) {
+ Logger.log(e);
+ }
+ });
+ thread.start();
+ return thread;
+}Let's verify the process execution behavior in similar files:
bundles/com.espressif.idf.core/src/com/espressif/idf/core/ProcessBuilderFactory.java (2)
51-51: LGTM! Clean and concise process stream handling.
The code cleanup improves readability while maintaining the correct process stream handling functionality.
39-42: Verify Windows compatibility for PATH handling.
While standardizing on "PATH" is good practice, removing "Path" on Windows systems needs careful consideration. Please ensure:
- The "PATH" variable is properly populated before removing "Path"
- This change doesn't affect CMake's ability to find required tools on Windows
Consider adding a more detailed comment explaining:
- Why we're removing "Path"
- How this relates to cross-platform compatibility
- Any prerequisites for PATH setup
✅ Verification successful
Let me analyze the search results to verify the PATH handling concern. I'll need to check how the environment variables are handled across the codebase, particularly in relation to the ProcessBuilderFactory.
Let me check one more aspect to understand the complete environment variable handling flow, particularly focusing on how the environment is populated before the Path removal.
Based on the comprehensive analysis of the codebase, I can now provide a definitive response about the PATH handling concern.
The PATH handling in ProcessBuilderFactory is correct and safe.
The code follows a deliberate standardization pattern where:
- The environment is first populated with all required variables via
processBuilder.environment().putAll(environment) - The Windows-specific "Path" is then removed because:
- The system environment is already properly captured through
IDFUtil.getSystemEnv() - The PATH variable (uppercase) contains the complete path information
- This standardization ensures consistent behavior across platforms
- The system environment is already properly captured through
The codebase shows this is part of a well-designed environment handling system where:
IDFUtil.getSystemEnv()properly captures all system environment variables- Multiple components (EspToolCommands, ExportIDFTools, etc.) use this same pattern
- The PATH/Path handling is consistently managed across the entire toolchain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other PATH/Path handling in the codebase
rg -i 'path.*=|path.*remove' --type java
# Look for CMake-related environment setup
rg -i 'cmake.*path|path.*cmake' --type java
Length of output: 91194
Script:
#!/bin/bash
# Search for environment variable handling in ProcessBuilderFactory and related classes
ast-grep --pattern 'processBuilder.environment().$_($_)'
# Look for PATH/Path environment variable usage in the codebase
ast-grep --pattern '$_.environment().get("PATH")'
ast-grep --pattern '$_.environment().get("Path")'
Length of output: 1100
Script:
#!/bin/bash
# Look for the environment setup before ProcessBuilder creation
rg -B 5 -A 5 'processBuilder.environment\(\).remove\("Path"\)' --type java
# Check for environment variable setup in IDFUtil
rg -A 5 'getSystemEnv' --type java
Length of output: 35202
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java (1)
118-121: LGTM: Project field usage is well-structured.
The conversion of the local project variable to a class field is a good improvement, making it accessible for both the Clang file updates and the new reconfigure functionality.
bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java (1)
Line range hint 1-449: Verify CMake command execution setup.
The changes set up environment variables and working directory for terminal operations, which are prerequisites for running CMake commands. However, we should verify that these changes properly support the CMake initial build functionality mentioned in the PR title.
✅ Verification successful
Let me check the CMake command execution setup more thoroughly.
Based on the findings, I can now generate the final response:
Terminal connector changes correctly support CMake build functionality
The changes in IDFConsoleLauncherDelegate.java properly support CMake build operations by:
- Setting up CDT build environment variables needed for CMake execution
- Establishing correct working directory for build operations through
getWorkingDir() - Ensuring proper PATH setup with IDF extras needed for CMake tools
The terminal connector acts as an environment provider rather than directly executing CMake commands, which aligns with its architectural role. The actual CMake execution is handled by IDFBuildConfiguration which uses this environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CMake-related functionality
# Test 1: Check for CMake command execution in the codebase
echo "Checking for CMake command execution..."
rg -l "cmake|CMake" --type java
# Test 2: Check for IDF reconfigure command usage
echo "Checking for reconfigure command usage..."
rg -l "reconfigure" --type java
Length of output: 4052
Script:
#!/bin/bash
# Check for CMake command execution in IdfCommandExecutor
echo "Checking IdfCommandExecutor for CMake commands..."
rg -A 10 "cmake" bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IdfCommandExecutor.java
# Check for CMake build configuration
echo "Checking CMake build configuration..."
rg -A 10 "cmake" bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java
# Check terminal connector's role in build process
echo "Checking terminal connector's role..."
rg "build|cmake" bundles/com.espressif.idf.terminal.connector/src/com/espressif/idf/terminal/connector/launcher/IDFConsoleLauncherDelegate.java
Length of output: 5956
| public static MessageConsole getConsole(String consoleName) | ||
| { | ||
| MessageConsole console = findConsole(consoleName); | ||
| if (console == null) | ||
| { | ||
| console = new MessageConsole(consoleName, null); | ||
| ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console }); | ||
| } | ||
| ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console); | ||
| return console; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and parameter validation.
The method logic is correct, but could benefit from these improvements:
- Add Javadoc with parameter and return descriptions
- Add null check for consoleName parameter
+/**
+ * Gets or creates a MessageConsole with the specified name.
+ * If a console with the given name already exists, it will be returned.
+ * Otherwise, a new console will be created and added to the console manager.
+ *
+ * @param consoleName the name of the console to get or create
+ * @return the MessageConsole instance
+ * @throws IllegalArgumentException if consoleName is null or empty
+ */
public static MessageConsole getConsole(String consoleName)
{
+ if (consoleName == null || consoleName.trim().isEmpty()) {
+ throw new IllegalArgumentException("Console name cannot be null or empty");
+ }
MessageConsole console = findConsole(consoleName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static MessageConsole getConsole(String consoleName) | |
| { | |
| MessageConsole console = findConsole(consoleName); | |
| if (console == null) | |
| { | |
| console = new MessageConsole(consoleName, null); | |
| ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console }); | |
| } | |
| ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console); | |
| return console; | |
| } | |
| /** | |
| * Gets or creates a MessageConsole with the specified name. | |
| * If a console with the given name already exists, it will be returned. | |
| * Otherwise, a new console will be created and added to the console manager. | |
| * | |
| * @param consoleName the name of the console to get or create | |
| * @return the MessageConsole instance | |
| * @throws IllegalArgumentException if consoleName is null or empty | |
| */ | |
| public static MessageConsole getConsole(String consoleName) | |
| { | |
| if (consoleName == null || consoleName.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Console name cannot be null or empty"); | |
| } | |
| MessageConsole console = findConsole(consoleName); | |
| if (console == null) | |
| { | |
| console = new MessageConsole(consoleName, null); | |
| ConsolePlugin.getDefault().getConsoleManager().addConsoles(new IConsole[] { console }); | |
| } | |
| ConsolePlugin.getDefault().getConsoleManager().showConsoleView(console); | |
| return console; | |
| } |
| private IStatus runIdfReconfigureCommand(IProject project) | ||
| { | ||
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | ||
| List<String> arguments = prepareArguments(); | ||
| Map<String, String> environment = new HashMap<>(new IDFEnvironmentVariables().getEnvMap()); | ||
|
|
||
| try (MessageConsoleStream messageConsoleStream = console.newMessageStream()) | ||
| { | ||
| return runProcess(arguments, environment, processRunner, project, messageConsoleStream); | ||
| } | ||
| catch (IOException e1) | ||
| { | ||
| Logger.log(e1); | ||
| return IDFCorePlugin.errorStatus(e1.getMessage(), e1); | ||
| } | ||
| } | ||
|
|
||
| private List<String> prepareArguments() | ||
| { | ||
| List<String> arguments = new ArrayList<>(); | ||
| arguments.add(IDFUtil.getIDFPythonEnvPath()); | ||
| arguments.add(IDFUtil.getIDFPythonScriptFile().getAbsolutePath()); | ||
| arguments.add("-DIDF_TARGET=" + target); //$NON-NLS-1$ | ||
| arguments.add("reconfigure"); //$NON-NLS-1$ | ||
| return arguments; | ||
| } |
There was a problem hiding this comment.
Add input validation and path verification.
The current implementation has several potential issues:
- Missing validation for the project parameter
- No verification if required paths (Python env, script file) exist
- Potential null pointer issues from utility methods
Consider adding these safety checks:
private IStatus runIdfReconfigureCommand(IProject project)
{
+ if (project == null || !project.exists()) {
+ return IDFCorePlugin.errorStatus("Invalid project", null);
+ }
+
ProcessBuilderFactory processRunner = new ProcessBuilderFactory();
List<String> arguments = prepareArguments();
Map<String, String> environment = new HashMap<>(new IDFEnvironmentVariables().getEnvMap());
private List<String> prepareArguments()
{
List<String> arguments = new ArrayList<>();
+ String pythonEnvPath = IDFUtil.getIDFPythonEnvPath();
+ if (pythonEnvPath == null || pythonEnvPath.isEmpty()) {
+ throw new IllegalStateException("Python environment path not found");
+ }
+ java.io.File scriptFile = IDFUtil.getIDFPythonScriptFile();
+ if (scriptFile == null || !scriptFile.exists()) {
+ throw new IllegalStateException("IDF Python script file not found");
+ }
- arguments.add(IDFUtil.getIDFPythonEnvPath());
- arguments.add(IDFUtil.getIDFPythonScriptFile().getAbsolutePath());
+ arguments.add(pythonEnvPath);
+ arguments.add(scriptFile.getAbsolutePath());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private IStatus runIdfReconfigureCommand(IProject project) | |
| { | |
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | |
| List<String> arguments = prepareArguments(); | |
| Map<String, String> environment = new HashMap<>(new IDFEnvironmentVariables().getEnvMap()); | |
| try (MessageConsoleStream messageConsoleStream = console.newMessageStream()) | |
| { | |
| return runProcess(arguments, environment, processRunner, project, messageConsoleStream); | |
| } | |
| catch (IOException e1) | |
| { | |
| Logger.log(e1); | |
| return IDFCorePlugin.errorStatus(e1.getMessage(), e1); | |
| } | |
| } | |
| private List<String> prepareArguments() | |
| { | |
| List<String> arguments = new ArrayList<>(); | |
| arguments.add(IDFUtil.getIDFPythonEnvPath()); | |
| arguments.add(IDFUtil.getIDFPythonScriptFile().getAbsolutePath()); | |
| arguments.add("-DIDF_TARGET=" + target); //$NON-NLS-1$ | |
| arguments.add("reconfigure"); //$NON-NLS-1$ | |
| return arguments; | |
| } | |
| private IStatus runIdfReconfigureCommand(IProject project) | |
| { | |
| if (project == null || !project.exists()) { | |
| return IDFCorePlugin.errorStatus("Invalid project", null); | |
| } | |
| ProcessBuilderFactory processRunner = new ProcessBuilderFactory(); | |
| List<String> arguments = prepareArguments(); | |
| Map<String, String> environment = new HashMap<>(new IDFEnvironmentVariables().getEnvMap()); | |
| try (MessageConsoleStream messageConsoleStream = console.newMessageStream()) | |
| { | |
| return runProcess(arguments, environment, processRunner, project, messageConsoleStream); | |
| } | |
| catch (IOException e1) | |
| { | |
| Logger.log(e1); | |
| return IDFCorePlugin.errorStatus(e1.getMessage(), e1); | |
| } | |
| } | |
| private List<String> prepareArguments() | |
| { | |
| List<String> arguments = new ArrayList<>(); | |
| String pythonEnvPath = IDFUtil.getIDFPythonEnvPath(); | |
| if (pythonEnvPath == null || pythonEnvPath.isEmpty()) { | |
| throw new IllegalStateException("Python environment path not found"); | |
| } | |
| java.io.File scriptFile = IDFUtil.getIDFPythonScriptFile(); | |
| if (scriptFile == null || !scriptFile.exists()) { | |
| throw new IllegalStateException("IDF Python script file not found"); | |
| } | |
| arguments.add(pythonEnvPath); | |
| arguments.add(scriptFile.getAbsolutePath()); | |
| arguments.add("-DIDF_TARGET=" + target); //$NON-NLS-1$ | |
| arguments.add("reconfigure"); //$NON-NLS-1$ | |
| return arguments; | |
| } |
| Job job = new Job(Messages.IdfReconfigureJobName) | ||
| { | ||
|
|
||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| IdfCommandExecutor executor = new IdfCommandExecutor(target, | ||
| ConsoleManager.getConsole("CDT Build Console")); //$NON-NLS-1$ | ||
| IStatus status = executor.executeReconfigure(project); | ||
| try | ||
| { | ||
| IDEWorkbenchPlugin.getPluginWorkspace().getRoot().refreshLocal(IResource.DEPTH_INFINITE, null); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return status; | ||
| } | ||
| }; | ||
| job.schedule(); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Console name should be defined as a constant and potentially externalized
The hardcoded console name "CDT Build Console" is used consistently across the codebase, but it should be defined as a constant in a central location since it's a shared resource identifier. The console name appears in both production and test code, making it more critical to maintain consistency through a single source of truth.
Consider:
- Define the console name as a constant in a shared location (e.g.,
UIPluginor a dedicated constants class) - Use the constant instead of the hardcoded string
- Consider externalizing the string to support internationalization if needed
Example refactoring:
// In UIPlugin or a dedicated constants class
public static final String CDT_BUILD_CONSOLE_NAME = "CDT Build Console"; //$NON-NLS-1$
// In NewIDFProjectWizard.java
ConsoleManager.getConsole(UIPlugin.CDT_BUILD_CONSOLE_NAME)🔗 Analysis chain
Several improvements needed in the reconfigure job implementation.
- The job scheduling in a dispose listener could lead to race conditions if the wizard is closed before the project is fully created.
- The error handling could be improved to provide better user feedback.
- The console name is hardcoded.
- The progress monitor is not being utilized.
Consider implementing these improvements:
-Job job = new Job(Messages.IdfReconfigureJobName)
+private static final String CDT_BUILD_CONSOLE = "CDT Build Console";
+
+Job job = new Job(Messages.IdfReconfigureJobName) {
+ @Override
+ protected IStatus run(IProgressMonitor monitor) {
+ try {
+ monitor.beginTask(Messages.IdfReconfigureJobName, IProgressMonitor.UNKNOWN);
+
+ if (project == null || !project.exists()) {
+ return new Status(IStatus.ERROR, UIPlugin.PLUGIN_ID,
+ "Project not found or not fully created");
+ }
+
+ IdfCommandExecutor executor = new IdfCommandExecutor(target,
+ ConsoleManager.getConsole(CDT_BUILD_CONSOLE));
+ IStatus status = executor.executeReconfigure(project);
+
+ if (monitor.isCanceled()) {
+ return Status.CANCEL_STATUS;
+ }
+
+ try {
+ IDEWorkbenchPlugin.getPluginWorkspace().getRoot()
+ .refreshLocal(IResource.DEPTH_INFINITE, monitor);
+ } catch (CoreException e) {
+ Logger.log(e);
+ return new Status(IStatus.ERROR, UIPlugin.PLUGIN_ID,
+ "Failed to refresh workspace", e);
+ }
+
+ return status;
+ } finally {
+ monitor.done();
+ }
+ }
+};
+job.setUser(true); // Show progress to user
job.schedule();Let's verify the console name usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded console names to ensure consistency
rg -l "CDT Build Console"
Length of output: 467
Script:
#!/bin/bash
# Let's check the actual usage context of "CDT Build Console" in these files
rg "CDT Build Console" -C 2
# Also check if there's any constant defined for this console name
rg -i "console.*constant|constant.*console"
Length of output: 12234
Script:
#!/bin/bash
# Let's check if there are any other console names used with ConsoleManager.getConsole
rg "ConsoleManager\.getConsole" -C 2
# Also check for any constants or messages related to CDT Build Console
rg -l "CDT.*Build.*Console" bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/messages/
Length of output: 877
| if (!StringUtil.isEmpty(idfExtraPaths)) { | ||
| envValue = envValue + ":" + idfExtraPaths; //$NON-NLS-1$ | ||
| } | ||
| } | ||
| envpList.add(envKey + "=" + envValue); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| //Removing path, since we are using PATH | ||
| envMap.remove("Path"); //$NON-NLS-1$ |
There was a problem hiding this comment.
Fix path separator and environment variable handling issues.
Several issues need to be addressed:
- The path separator ":" is hardcoded, which won't work on Windows. Use
File.pathSeparatorinstead. - The removal of "Path" after adding all environment variables to
envpListis redundant and potentially problematic.
Apply this diff to fix the issues:
if (!StringUtil.isEmpty(idfExtraPaths)) {
- envValue = envValue + ":" + idfExtraPaths; //$NON-NLS-1$
+ envValue = envValue + File.pathSeparator + idfExtraPaths;
}Also, consider moving the envMap.remove("Path") before the environment variables are added to envpList to ensure consistency.
Committable suggestion skipped: line range outside the PR's diff.
sigmaaa
changed the title
|
@alirana01 @kolipakakondal hi guys! |
Description
Running the idf.py reconfigure command after creating a new project. After that the sdkconfig should appear in the project without executing full build. Also, the problem related to executing idf.py commands on windows without previously building the project is fixes in this PR
Fixes # (IEP-1247)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Bug Fixes