feat(tools):Use Windows native path separator in ESP_SR model copy command pattern #9649
Conversation
This file contains 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
👋 Hello per1234, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Although Windows generally supports the use of the POSIX compliant slash path separator in addition to the Windows native backslash separator, in the specific use case where a path is present in a native command executed via an argument to `cmd /c` in a platform command pattern, it is mandatory to use backslash path separators. Previously, a slash path separator was used in the `tools.esp32-arduino-libs.path` and `compiler.sdk.path` platform properties, which were referenced in a `copy` command in the `cmd /c` argument part of the platform's `recipe.hooks.objcopy.postobjcopy.2.pattern.windows` command pattern. This caused compilation to fail with a "The syntax of the command is incorrect." error under the following conditions: - The compilation is performed on a Windows machine - The compiled sketch uses the ESP_SR library This is fixed by adding Windows override variants of the properties, with backslash path separators.
me-no-dev
reviewed
May 17, 2024
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
VojtechBartoska
changed the title
|
Hello @per1234, any updates regarding this PR up to the latest review? Thanks a lot in advance! |
|
@VojtechBartoska the PR stands on its own as it is now. It fixes a serious bug without introducing any defects. So if you like, you can merge it as it is now. If you want me to add the |
|
I see, thanks for clarification @per1234. Do you mind if we take over of this PR and commit requested changes? |
Not at all. Feel free to do so. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Although Windows generally supports the use of the POSIX compliant slash (
/) path separator in addition to the Windows native backslash (\) separator, in the specific use case where a path is present in a native command executed via an argument tocmd /cin a platform command pattern, it is mandatory to use backslash path separators.Previously, a slash path separator was used in the
tools.esp32-arduino-libs.path(non-release value) andcompiler.sdk.pathplatform properties, which were referenced in acopycommand in thecmd /cargument part of the platform'srecipe.hooks.objcopy.postobjcopy.2.pattern.windowscommand pattern. This caused compilation to fail with a "The syntax of the command is incorrect." error under the following conditions:Description of Change
Fix the compilation failure by adding Windows override variants of the properties, with backslash path separators.
Tests scenarios
I have tested my pull request by compiling the following minimal sketch on a Windows machine:
Prior (50ef6f4) to the change proposed by this PR, compilation of this sketch failed:
(note the
/in"C:\\Users\\per\\AppData\\Local\\Arduino15\\packages\\esp32\\tools\\esp32-arduino-libs\\idf-release_v5.1-442a798083/esp32s3\\esp_sr\\srmodels.bin")After the change proposed by this PR, the compilation is successful, with the
recipe.hooks.objcopy.postobjcopy.2.pattern.windowscommand pattern now generating a command with this format:(note the path is now
"C:\\Users\\per\\AppData\\Local\\Arduino15\\packages\\esp32\\tools\\esp32-arduino-libs\\idf-release_v5.1-442a798083\\esp32s3\\esp_sr\\srmodels.bin", with exclusively\path separators)I verified the results with Arduino IDE 2.3.2 and 1.8.19, with both a manual installation of the platform as well as the Boards Manager installation (with the
sedprocessing ofplatform.txtprocessing that is applied by.github/scripts/on-release.sh).Related links
Originally reported at https://forum.arduino.cc/t/esp32-exit-status-1-when-building-esp-sr-basic-example/1260494