Skip to content
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

Enhance/whitespaces2 #1159

Merged
merged 18 commits into from
Jun 7, 2024
Merged

Enhance/whitespaces2 #1159

merged 18 commits into from
Jun 7, 2024

Conversation

radurentea
Copy link
Collaborator

@radurentea radurentea commented Mar 10, 2024

Description

Enable usage of white spaces in project's path, esp idf's path and tools's path.
https://jira.espressif.com:8443/browse/VSC-808

Fixes #1162
Fixes #939

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Steps to test this pull request

Provide a list of steps to test changes in this PR and required output

  1. Configure extension with whitespaces in esp idf's path, in the tools' path and project's path.
  2. Test all features (building, flashing, monitoring etc)

All features should work, however, there is an issue currently with esp-idf while building a project with whitespaces:

 Run Build Command(s):C:/ESPRES~2/tools/ninja/111~1.1/ninja.exe cmTC_c5154 && [1/2] Building C object CMakeFiles/cmTC_c5154.dir/testCCompiler.c.obj
    FAILED: CMakeFiles/cmTC_c5154.dir/testCCompiler.c.obj 
    C:\ESPRES~2\tools\XTENSA~2\ESP-13~1.0_2\XTENSA~1\bin\XT8994~1.EXE   -mlongcalls -Wno-frame-address -o CMakeFiles/cmTC_c5154.dir/testCCompiler.c.obj -c "C:/Users/Radu/Desktop/esp projects/esp     spaces/blink/build/CMakeFiles/CMakeTmp/testCCompiler.c"
    thread 'main' panicked at 'assertion failed: `(left == right)`
      left: `"XT8994~1.EXE"`,
     right: `"xtensa"`: Called tool must have pattern "xtensa-esp*-elf-*"', main.rs:46:18
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
    ninja: build stopped: subcommand failed.

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Tested as described above on Windows 11 with different shells, including: Command Prompt, Powershell and Bash

Test Configuration:

  • ESP-IDF Version: 5.2.1
  • OS (Windows,Linux and macOS): Windows

Checklist

  • PR Self Reviewed
  • Applied Code formatting
  • Added Documentation
  • Added Unit Test
  • Verified on all platforms - Windows,Linux and macOS

@radurentea radurentea added the ongoing Ongoing Issue or PR, this label will be used for issue or PR which is to be excluded by stale bot label Mar 10, 2024
Copy link

github-actions bot commented Mar 16, 2024

Download the artifacts for this pull request:

@radurentea radurentea self-assigned this Apr 4, 2024
@radurentea radurentea marked this pull request as ready for review April 4, 2024 13:05
Copy link
Collaborator

@brianignacio5 brianignacio5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to rebase this branch to current master to remove changes that doesn't belong to the PR and keep changes related to feature being introduced.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@brianignacio5
Copy link
Collaborator

Need to update the monitor code because this doesn't work.

PS C:\Users\brian\workspace\test\v52\whitespace example> "c:\Users\brian\.espressif\python_env\idf5.2_py3.11_env\Scripts\python.exe" "c:\Users\brian\esp\space example\v5.2.1\esp-idf\tools\idf_monitor.py" 
-p COM8 -b 115200 --toolchain-prefix xtensa-esp32-elf- --target esp32 "c:\Users\brian\workspace\test\v52\whitespace exam52\whitespace example\build\whitespace.elf"
At line:1 char:77
+ ... python.exe" "c:\Users\brian\esp\space example\v5.2.1\esp-idf\tools\id ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Unexpected token '"c:\Users\brian\esp\space example\v5.2.1\esp-idf\tools\idf_monitor.py"' in 
expression or statement.
At line:1 char:148
+ ... ian\esp\space example\v5.2.1\esp-idf\tools\idf_monitor.py" -p COM8 -b ...
+                                                                ~~
Unexpected token '-p' in expression or statement.
At line:1 char:151
+ ... sp\space example\v5.2.1\esp-idf\tools\idf_monitor.py" -p COM8 -b 1152 ...
+                                                              ~~~~
Unexpected token 'COM8' in expression or statement.
    + CategoryInfo          : ParserError: (:) [], ParentContainsErrorRecordException
    + FullyQualifiedErrorId : UnexpectedToken

@brianignacio5
Copy link
Collaborator

PTAL @AndriiFilippov

- Replaced Shell Executions task type with Process Executions
- Modified a bit the code for consistency in the project (replacing, workspace variable name to workspaceUri because there is a vscode.workspace an this two can be confused if we import "workspace" from "vscode" specifically.
From workspace to workspaceUri
TODO: UI changes to let user know it only work on version of esp-idf 5.0 and higher
Revert "Change node.processChild.exec to .execFile"

This reverts commit b703c26.

Revert "Revert "Change node.processChild.exec to .execFile""

This reverts commit 84fcba5.
Fix typo for espBom

:art: Fix formatting
🎨 Fix formatting
Use CMake Bin Path for building
If user doesn't have default terminal set, we just use the default ones for each os
@AndriiFilippov
Copy link
Collaborator

@radurentea hi !

Tested under:
OS - Windows 10
ESP-IDF: v5.2.1
Throws error message about ESP-IDF path.
image

@radurentea
Copy link
Collaborator Author

Hi @AndriiFilippov,

The error you receive is because idf tools' path contains whitespaces and currently whitespaces can only be present in esp idf and project's path.

However, it seems that we show all errors under esp-idf path.

I will work on a fix so that user sees the error under tools' path and also if errors are being displayed the install button will be disabled.

Copy link
Collaborator

@brianignacio5 brianignacio5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doc comment but otherwise LGTM

docs/tutorial/install.md Show resolved Hide resolved
@radurentea
Copy link
Collaborator Author

@AndriiFilippov PTAL, I've added UI Validation.
If user doesn't have any esp-idf version selecting, he will receive a warning in case he has whitespaces in idf path.
If user have white spaces in tools path or he selects an esp-idf version lower than 5, he will receive error and won't be able to continue with the install.

@AndriiFilippov
Copy link
Collaborator

@radurentea hi !

Tested under:
OS - Windows 10

Tools PATH validation ✔️
ESP-IDF PATH support spaces ✔️
Project name support spaces ✔️

able to create / build / flash / monitor / debug project ✔️

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ongoing Ongoing Issue or PR, this label will be used for issue or PR which is to be excluded by stale bot
Projects
None yet
3 participants