IEP-1472 Custom build folder: Whitespace in build folder path lead to error #1193
Conversation
WalkthroughThe pull request streamlines build folder configuration throughout the codebase. In the core build configuration, it removes legacy handling for custom build directories and simplifies the logic to always use the default build directory. A new constant is added to the launch constants. The launch configuration UI is enhanced by introducing a new UI component for selecting the build folder, along with updated persistence logic. Additionally, related user messages and localization entries are expanded to support the changes. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant TabGroup as SerialFlashLaunchConfigTabGroup
participant Config as ILaunchConfigurationWorkingCopy
participant Delegate as CoreBuildLaunchConfigDelegate
participant Util as IDFUtil
participant Project as IProject
participant Logger
UI->>+TabGroup: performApply(configuration)
TabGroup->>+Delegate: getProject(configuration)
Delegate-->>-TabGroup: project / null
alt project is null
TabGroup->>Logger: Log error and exit
else project exists
TabGroup->>+Config: Retrieve build folder attribute
Config-->>-TabGroup: build folder value (or blank)
alt build folder is blank or relative
TabGroup->>+Util: getBuildDir(project)
Util-->>-TabGroup: default build path
TabGroup->>Project: Adjust build folder to absolute path
end
TabGroup->>+Util: setBuildDir(project, buildFolder)
Util-->>-TabGroup: Confirm directory setup
Note right of TabGroup: Exceptions caught and logged via Logger.log(e)
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
158-166: Simplified build path handling improves whitespace supportThe changes to
getBuildContainerPath()simplify the code by removing custom build directory parsing logic and centralizing the build directory handling throughIDFUtil.getBuildDir(). This aligns well with the PR objective of addressing whitespace issues in build folder paths.I'd suggest adding a null check before calling methods on the path to avoid potential NullPointerExceptions:
public IPath getBuildContainerPath() throws CoreException { org.eclipse.core.runtime.Path path = new org.eclipse.core.runtime.Path(IDFUtil.getBuildDir(getProject())); - if (!path.toFile().exists()) + if (path != null && !path.toFile().exists()) { path.toFile().mkdirs(); } return path; }bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java (4)
53-53: Consider documenting the reason for changing DEFAULT_CMAKE_MSG to an empty string.This change from a potentially informative default message to an empty string might impact user experience. Consider adding a comment explaining the rationale for this change.
200-202: Consider adding validation for the build folder path.While the implementation correctly retrieves and sets the build folder path, consider adding validation to ensure the path is valid and accessible.
String buildFolderPath = configuration.getAttribute(IDFLaunchConstants.BUILD_FOLDER_PATH, StringUtil.EMPTY); +// Validate the build folder path +if (!buildFolderPath.isEmpty() && !Path.of(buildFolderPath).toFile().exists()) { + // Log a warning or display a message + Logger.log("Build folder path does not exist: " + buildFolderPath); +} buildFolderText.setText(buildFolderPath);
251-251: Consider trimming the build folder path before saving.To avoid issues with leading/trailing whitespace (which was the original problem this PR addresses), it's a good practice to trim the text input before saving.
-configuration.setAttribute(IDFLaunchConstants.BUILD_FOLDER_PATH, buildFolderText.getText()); +configuration.setAttribute(IDFLaunchConstants.BUILD_FOLDER_PATH, buildFolderText.getText().trim());
297-297: Maintain consistency with trimming across the codebase.Similar to the previous comment about line 251, you're correctly using trim() here but not in the earlier instance. Ensure consistency across the codebase.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (1)
70-70: Consider using StringUtil.isEmpty instead of isBlank.For consistency with other parts of the codebase (e.g., IDFUtil.getBuildDir), consider using StringUtil.isEmpty() instead of the String.isBlank() method.
-buildFolder = buildFolder.isBlank() ? IDFUtil.getBuildDir(project) : buildFolder; +buildFolder = StringUtil.isEmpty(buildFolder) ? IDFUtil.getBuildDir(project) : buildFolder;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java(2 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java(9 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java(1 hunks)bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(56-839)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-21)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(56-839)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (6)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
20-20: Good addition of new launch constant for build folder pathThis new constant will provide a consistent way to reference the build folder path throughout the codebase, which supports the PR objective of enhancing build folder management.
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/Messages.java (1)
14-17: Well-structured message constants for build folder UIThese new message constants follow the existing naming convention and provide localization support for the new build folder selection UI, which aligns with the PR objective of adding a new field in the Build tab.
Also applies to: 21-21
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/messages.properties (1)
6-8: Clear and helpful user messages for build folder selectionThe added messages provide clear guidance to users about the purpose of the build folder field and how to use it. The tooltip and message having identical text is appropriate for consistency.
Also applies to: 13-13
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/dialogs/CMakeBuildTab2.java (2)
112-142: Great job implementing the build folder selection UI!The implementation follows UI design best practices with:
- Clear labeling and informative tooltip
- Proper layout and spacing
- Reuse of existing patterns (createPushButton)
- Appropriate event handling
The directory dialog implementation is well-structured with clear messaging.
324-328: LGTM - Good handling of nullable property.The code correctly checks if the property is null before attempting to set the text field, which prevents potential NullPointerExceptions.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (1)
57-81: Great implementation of the performApply method!The method effectively:
- Retrieves the build folder path from configuration (with fallback)
- Checks if the path is empty and handles accordingly
- Converts relative paths to absolute paths
- Persists the build folder path using IDFUtil
- Includes proper error handling
This implementation ensures the build folder path is correctly handled regardless of whether it contains whitespace, addressing the original issue described in the PR objectives.
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Looks good, @AndriiFilippov II think we need to verify user cases, such as what happens if the user provides -B dev_build in the Additional Arguments section as well as in the newly introduced Build Directory section.
|
@sigmaaa hi! Tested under:
build project - go again to "Build folder location", remove path and save changes to Launch Config + delete this folder build the project -> it still builds to the previously removed path. If I create NEW Launch Config - it still build project to the removed PATH from previous Launch Config. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (1)
58-82: Consider adding null check for buildFolder to prevent potential NPE.The implementation correctly applies build folder settings to the project, but there's a potential NullPointerException if
buildFolderis null before checkingisBlank(). While this is unlikely givenIDFUtil.getBuildDir()implementation, defensive programming would add an explicit null check.Additionally, consider adding a comment explaining the purpose of this method for future maintainers, especially regarding the path manipulation logic.
@Override public void performApply(ILaunchConfigurationWorkingCopy configuration) { super.performApply(configuration); try { IProject project = CoreBuildLaunchConfigDelegate.getProject(configuration); if (project == null) { return; } String buildFolder = configuration.getAttribute(IDFLaunchConstants.BUILD_FOLDER_PATH, IDFUtil.getBuildDir(project)); - buildFolder = buildFolder.isBlank() ? IDFConstants.BUILD_FOLDER : buildFolder; + buildFolder = (buildFolder == null || buildFolder.isBlank()) ? IDFConstants.BUILD_FOLDER : buildFolder; if (!Path.of(buildFolder).isAbsolute()) { buildFolder = project.getLocation().append(buildFolder).toOSString(); } IDFUtil.setBuildDir(project, buildFolder); } catch (CoreException e) { Logger.log(e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-21)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
IDFUtil(56-839)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (3)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/SerialFlashLaunchConfigTabGroup.java (3)
18-37: Import statements adequately support the added functionality.The new imports are appropriate for supporting the build folder path functionality, including the Path class for path manipulation and the necessary Eclipse framework classes.
71-75: Path handling logic addresses whitespace issues correctly.The code correctly handles relative and absolute paths, ensuring that the build folder path is always absolute before setting it in the project properties. This approach effectively addresses the whitespace issue mentioned in the PR objectives.
58-82:❓ Verification inconclusive
Verify handling of removed build folder paths.
According to the PR comments, there was an issue where "selecting a build folder using the new 'Build folder location' field, then attempting to remove the path and save changes" still resulted in building to the previously removed path.
This implementation treats blank paths as a reset to the default build folder, but you should verify this behavior matches the expected user experience.
Run this script to help understand how the user interface and this code interact:
🏁 Script executed:
#!/bin/bash # Search for UI components that might clear or set the build folder path rg -A 5 -B 5 "BUILD_FOLDER_PATH" --glob="*.java" # Check if any UI components handle cleared paths differently rg -A 3 "isBlank|isEmpty" --glob="*.java" | grep -i "buildFolder|buildDir|build.*folder|build.*path"Length of output: 8479
Action Required: Verify that a cleared build folder path properly resets to the default
The implementation in
SerialFlashLaunchConfigTabGroup.javanow checks for a blank build folder string and substitutes it withIDFConstants.BUILD_FOLDER. This should address the issue where a previously selected custom build folder persisted after the field was cleared. However, please ensure that the UI (e.g., inCMakeBuildTab2.java) indeed updates the configuration attribute to an empty string when the user removes any value from the "Build folder location" field. Manual verification of this interaction is advised to confirm that the expected behavior is reached in practice.
- Confirm that clearing the Build folder field in the UI results in an empty (or blank) string being set.
- Verify that the fallback logic in
performApplycorrectly resets the build folder toIDFConstants.BUILD_FOLDERin such cases.
|
Hi @AndriiFilippov, Thanks for verifying the PR.
This should be fixed in the last commit
That’s not entirely true. A separate field for setting up the build folder offers a more user-friendly experience. It provides visual clarity and reduces errors, as users lack the auto-complete feature typically available in the CLI. On the development side, it’s also easier to maintain a dedicated field, rather than relying on a multi-purpose field that could contain spaces and special characters, which can make parsing more complicated. |
|
Hi @AndriiFilippov, I've also noticed that the idf.py reconfigure command still relies on the -B argument, so I will change it in the next commit. |
|
Fixed an issue where the IDF command executor was relying on its own method to retrieve the custom build folder. Additionally, enhanced the update process for the project's build folder path based on the active launch configuration." |
|
@sigmaaa hi ! use just getting this error: same if both fields filled with the paths |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (1)
487-493: Consider using regex pattern match for more precise -B argument detection.The current implementation uses a simple string contains check which could potentially match other arguments or text that happens to contain "-B" but aren't actually the build directory argument.
- if (cmakeArgs.contains("-B")) //$NON-NLS-1$ + import java.util.regex.Pattern; + // Add import at the top of the file + + // In the method: + if (Pattern.compile("\\b-B\\b").matcher(cmakeArgs).find())This regex ensures we're only matching the exact "-B" flag as a word boundary and not part of another string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java(2 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/messages.properties(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/CMakeMainTab2.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java (1)
Messages(20-73)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (2)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/Messages.java (1)
33-33: LGTM: New message for deprecated -B argument added.The new message variable follows the established naming convention and pattern for message declarations in the class.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/messages.properties (1)
21-21: LGTM: Clear and actionable error message added.The new message provides users with a clear explanation that the -B argument is deprecated and instructs them to use the Build location setting instead.
|
Hi @AndriiFilippov, fixed in the last commit by validating the CMake arguments field and not letting use -B arguments:
|
|
Hi @sigmaaa Please update the documentation for this |
Hi @kolipakakondal, done |
|
@sigmaaa hi ! The new field works as expected ✅ LGTM 👍 |
Description
To improve the user experience and maintain cleaner code when handling custom build folders, I’ve added a new field to the Build tab specifically for the build folder path.
Fixes # (IEP-1472)
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
Documentation