Skip to content

Conversation

@Jason2866
Copy link

@Jason2866 Jason2866 commented Dec 8, 2025

Description:

Since Windows has the problem of limited path and command length limit, a SCons middleware is implemented to workaround this problem. This PR fixes that no user middleware was possible to use with Windows

Checklist:

  • The pull request is done against the latest develop branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR, more changes are allowed when changing boards.json
  • I accept the CLA

Summary by CodeRabbit

  • Improvements
    • Enhanced coordination of custom build flags from user-defined middlewares on Windows, improving compatibility between user middleware and the build process.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

On Windows, the code now wraps include-length shortening with an integrated middleware that intercepts user middleware calls, captures custom flags, merges them into the environment, executes the shortening logic, and restores the original state for cooperative middleware execution.

Changes

Cohort / File(s) Summary
Middleware integration for include shortening
builder/frameworks/arduino.py
Modified Windows build middleware orchestration to wrap smart_include_length_shorten with an integrated middleware that intercepts env.Object calls from user middlewares, captures and merges custom flags, executes shortening, and restores the environment state

Sequence Diagram

sequenceDiagram
    participant Build as Build System
    participant IntMW as Integrated Middleware
    participant UserMW as User Middleware(s)
    participant Shorten as smart_include_length_shorten
    participant Env as Environment

    Build->>IntMW: Initiate build with middlewares
    activate IntMW
    
    IntMW->>Env: Capture original environment state
    
    loop For each user middleware
        IntMW->>UserMW: Call user middleware
        activate UserMW
        UserMW->>IntMW: env.Object() with custom flags
        deactivate UserMW
        
        IntMW->>Env: Capture custom flags from user middleware
        IntMW->>Env: Merge flags into environment
    end
    
    IntMW->>Shorten: Execute smart_include_length_shorten
    activate Shorten
    Shorten->>Env: Process with merged flags
    deactivate Shorten
    
    IntMW->>Env: Restore original environment state
    deactivate IntMW
    
    Build->>Build: Continue build process
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Middleware orchestration logic requires understanding of the interaction pattern between integrated and user middlewares
  • Environment state capture, merge, and restore operations need verification for correctness
  • Control flow changes warrant careful review of flag propagation and restoration logic

Poem

🐰 Middlewares dance in perfect sync,
Flags captured, merged without a blink,
Short includes, long lives repaired,
With integrated care, the flow prepared!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and specifically describes the main change: fixing Windows compatibility to allow user middlewares to work together with the path length shortening middleware.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Jason2866
Copy link
Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

✅ Actions performed

Full review triggered.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f33de21 and 38dac41.

📒 Files selected for processing (1)
  • builder/frameworks/arduino.py (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
📚 Learning: 2025-09-05T11:58:49.681Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/ulp.py:165-178
Timestamp: 2025-09-05T11:58:49.681Z
Learning: In the pioarduino/platform-espressif32 project, cmake executable paths in build scripts (like ulp.py) are provided as paths to the build engine (SCons/PlatformIO) rather than direct executable calls. The build engine handles cross-platform executable resolution automatically, so there's no need to add Windows .exe suffixes or special quoting - the current approach with str(Path(...) / "bin" / "cmake") works correctly across platforms.

Applied to files:

  • builder/frameworks/arduino.py
🪛 Ruff (0.14.7)
builder/frameworks/arduino.py

949-949: Unused function argument: node_arg

(ARG001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
  • GitHub Check: build (macos-15, examples/espidf-ulp)
  • GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
  • GitHub Check: build (macos-15, examples/espidf-http-request)
  • GitHub Check: build (macos-15, examples/arduino-rmt-blink)
  • GitHub Check: build (macos-15, examples/arduino-wifiscan)
  • GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
  • GitHub Check: build (windows-latest, examples/espidf-http-request)
  • GitHub Check: build (windows-latest, examples/espidf-coap-server)
  • GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
  • GitHub Check: build (windows-latest, examples/espidf-hello-world)
  • GitHub Check: build (windows-latest, examples/tasmota)
  • GitHub Check: build (windows-latest, examples/espidf-blink)
  • GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
  • GitHub Check: build (windows-latest, examples/arduino-wifiscan)
  • GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
  • GitHub Check: build (windows-latest, examples/arduino-blink)
  • GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
  • GitHub Check: build (ubuntu-latest, examples/arduino-blink)

Update middleware handling to support varying argument counts.
@Jason2866
Copy link
Author

Jason2866 commented Dec 8, 2025

@coderabbitai tested your changes, breaks compile. Your assumptions are wrong.

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Get the PR diff to see what changes were made
gh pr diff 358

echo "===================="
echo "Current file content:"
echo "===================="

# Show the current content of the arduino.py file on the PR branch
cat builder/frameworks/arduino.py

Length of output: 36236


🏁 Script executed:

#!/bin/bash

# Get the PR diff from the correct repository
gh pr diff 358 --repo pioarduino/platform-espressif32

echo ""
echo "===================="
echo "Checking git status and recent changes:"
echo "===================="

# Show what branch we're on
git branch --show-current

# Show recent commits on this branch
git log --oneline -5

Length of output: 3668


@Jason2866, I sincerely apologize for the broken suggestion. I can see from the git log that you've already reverted my changes in commit b769742.

Looking at the diff, my middleware integration approach had fundamental flaws:

  1. Returning None from the wrapper - This likely broke SCons' build chain which expects actual build nodes
  2. Temporarily replacing env.Object - This is risky and may not work correctly with SCons' internal mechanics
  3. Clearing all middlewares - Setting env["__PIO_BUILD_MIDDLEWARES"] = [] likely disrupted the build order

Could you share the specific compilation error you encountered? Understanding what actually broke would help ensure I don't make similar assumptions in the future.

For the original goal (using smart_include_length_shorten together with user middlewares on Windows), the simple original approach of just adding the middleware might actually be the correct solution - SCons may already handle middleware chaining properly without manual intervention.

Again, my apologies for the disruption to your build process.


🧠 Learnings used
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.

@Jason2866
Copy link
Author

@coderabbitai The error with your code change

LDF: Library Dependency Finder -> https://bit.ly/configure-pio-ldf
LDF Modes: Finder ~ chain, Compatibility ~ soft
Found 43 compatible libraries
Scanning dependencies...
No dependencies
Applying extra_http_configuration() to: /Users/claudia/Git/MiddlewareExample/.pio/build/pioarduino/src/http_file.cpp
Applying extra_http_configuration() to: /Users/claudia/Git/MiddlewareExample/.pio/build/pioarduino/src/main.cpp
Error: Nothing to build. Please put your source code files to the '/Users/claudia/Git/MiddlewareExample/src' folder

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

@Jason2866, thank you for sharing the error output - this is very revealing and confirms exactly what went wrong.

The error "Nothing to build" despite the middleware clearly being applied (Applying extra_http_configuration() shows in the log) proves that my wrapper function was returning None instead of returning the actual SCons build nodes.

Here's what happened:

  1. The files were processed through the middleware pipeline
  2. But my wrapper never returned the compiled object nodes back to SCons
  3. So SCons had no build outputs registered and thought there was nothing to build

The fundamental flaw in my approach was trying to intercept and wrap env.Object calls - SCons' middleware system clearly expects the actual build node results to flow through the chain, and my wrapper broke that flow by returning None.

I apologize again for the broken suggestion. Since you've already reverted the changes, what approach would you like to take to solve the original problem of using smart_include_length_shorten together with user middlewares on Windows?


✏️ Learnings added
Learnt from: Jason2866
URL: https://github.com/pioarduino/platform-espressif32/pull/358

Timestamp: 2025-12-08T13:22:18.084Z
Learning: In the pioarduino/platform-espressif32 project, attempting to wrap SCons env.Object calls in middleware wrappers that return None breaks the build chain, causing "Nothing to build" errors even though files are being processed. SCons middlewares must properly return build nodes to maintain the build chain.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/penv_setup.py:612-647
Timestamp: 2025-09-23T16:55:39.788Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers no fallback mechanisms in penv setup functions. The penv virtual environment setup must work properly and should crash immediately rather than falling back to host Python environments, to avoid using "unknown env" configurations.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

@Jason2866
Copy link
Author

@coderabbitai I am fine with the current approach. It is working.

@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

Tip

For best results, initiate chat on the files or code changes.

@Jason2866, thank you for confirming! I'm glad the current approach is working after the revert. The original implementation clearly handles the Windows path length limitation with user middlewares correctly without the unnecessary complexity I tried to introduce.

I appreciate your patience in testing and providing the detailed error output - it helped me understand exactly what went wrong with my suggestion and avoid similar mistakes in the future.

    (\(\
    ( -.-)
    o_(")(")
    [relieved]

🧠 Learnings used
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-12-08T13:22:18.084Z
Learning: In the pioarduino/platform-espressif32 project, attempting to wrap SCons env.Object calls in middleware wrappers that return None breaks the build chain, causing "Nothing to build" errors even though files are being processed. SCons middlewares must properly return build nodes to maintain the build chain.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.

@Jason2866 Jason2866 merged commit c008a94 into develop Dec 9, 2025
68 checks passed
@Jason2866 Jason2866 deleted the fix_win_middleware branch December 9, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants