fix Windows installer Scripts path detection - #107
Open
Dhananjay2799 wants to merge 2 commits into
Open
Conversation
srpatcha
reviewed
Sep 3, 2026
srpatcha
left a comment
srpatcha
left a comment
Member
There was a problem hiding this comment.
Review — ebuild#107 "fix Windows installer Scripts path detection"
head: f61353e author: Dhananjay2799 ci: none reported (checks.txt is 0 bytes)
Verdict: A real fix for a real bug. Both embedded Python commands in install.bat were syntax errors, so SCRIPTS_DIR was never set on any Windows machine, and the replacements are the correct API for what the originals were trying to compute. I confirmed the new test fails on master and passes here. The finding is that the test guards the easy half of this file and is structurally unable to see the half that actually breaks it — I demonstrated that by breaking the other half and watching it pass.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_windows_installer.py:11-30 |
The regression test checks the Python fragment and is blind to the batch syntax around it, which is where this file's risk lives. It slices line.index('-c "') … line.rfind('"') and calls compile() on the result — so it validates the Python in isolation and never looks at the for /f "delims=" %%i in ('…') do construct that delivers it. I took the patched tree, deleted the closing ' from line 55 and the opening ( from line 58 — two edits that would make cmd.exe fail outright — left the Python untouched, and re-ran: 1 passed. That matters more here than it would elsewhere, because the code being replaced was itself batch-quoting gymnastics: the originals wrote chr(92) and chr(39) specifically to keep literal backslashes and single quotes out of a for /f ('…'), and this PR removes that workaround and puts literal 'scripts' and 'nt_user' back in without saying why it is now safe. For what it is worth I think it is safe — cmd ignores parentheses inside double quotes when finding the closing ), and strips only the outermost ', so the inner quotes reach python intact — but that is me reasoning about cmd.exe from Linux, and it is exactly the kind of thing the test was added to stop being a matter of opinion. |
Extend the same string parsing to assert the batch shape, which is a few lines and needs no new dependency: for every line containing -c ", assert it matches for /f … in ('…') do, that the (' and ') are balanced, and that the -c " quote closes before the '). That turns "the Python compiles" into "the line is well-formed", which is the property that failed. If a stronger guarantee is wanted, a windows-latest job that runs only the detection block and echoes SCRIPTS_DIR would settle it for real — install.bat currently has no CI coverage at all (git grep install.bat -- .github/ returns nothing), so this test is the first guard it has ever had. |
| 2 | Medium | — (checks.txt is empty; PR body) |
No CI check ran, and the only evidence for a Windows-only change is one unquoted sentence. checks.txt is 0 bytes; createdAt and updatedAt are both 2026-09-02T15:35:59Z, opened and never touched, while ebuild#103 and #104 in this batch carry 24 and 30 checks. The body is the template with Summary left blank, "Related Issues" as N/A, and the evidence reduced to two ticked boxes ("Manual testing performed", "All existing tests pass") plus "Validated on Windows with Python 3.14.6 across active virtual environments and per-user installation paths" — no output, no path, no SCRIPTS_DIR value. Under the brief's rule that an unsupported "verified" is itself the finding, that sentence is it. Two further wrinkles: 3.14.6 is outside this repository's CI matrix (ci.yml tests 3.10, 3.11, 3.12) and well above requires-python = ">=3.8", so the one environment it was checked in is the one CI does not cover; and "Branch is rebased on latest master" is left unchecked. |
A maintainer approves the workflow runs. Replace the validation sentence with the actual terminal output — echo %SCRIPTS_DIR% from both the venv case and the per-user case is two lines and settles the whole question. Fill in the Summary; the "Changes" section already says what is needed and just needs moving up. |
| 3 | Low | install.bat:55-58 |
The two probes are in the less useful order. The per-user scheme (scheme='nt_user' → %APPDATA%\Python\PythonXY\Scripts) is tried first, and the default scheme — which resolves to the active virtualenv's Scripts, or the interpreter's own — only as the fallback when ebuild.exe is not found in the first. A developer running this inside an active venv, which the body names as one of the two validated cases, always misses on probe one and always lands on probe two, paying an extra interpreter start-up to do it. This is inherited from the original (getusersitepackages() first) so it is not a regression, and it is harmless — just backwards for the common case. |
Swap them: sysconfig.get_path('scripts') first, scheme='nt_user' as the fallback. One-line reorder, and it makes the venv case a single subprocess. |
| 4 | Low | tests/unit/test_windows_installer.py:1 |
The new file begins with a UTF-8 BOM. Python reads it as utf-8-sig so nothing breaks, but no other .py file in the repository has one — git grep -lI $'\xef\xbb\xbf' origin/master -- '*.py' returns zero — so this would be the first, and it is the kind of thing that produces confusing diffs later. Almost certainly an editor default rather than a decision. |
Save without the BOM. |
Verified clean, executed, because the value of this PR turns on whether the bug and the guard are both real:
- The bug was real and total.
install.bat:55onmastercontainschr(92)+chr(39)lib'+chr(92)+'site-packages, which isSyntaxError: unterminated string literal, and:58containschr(39)Scripts', the same. Bothfor /floops therefore produced no output andSCRIPTS_DIRwas left empty, so the PATH step this file exists for could never have worked on any Windows machine. This is not a corner case. - The new test catches exactly that. Applied the patch to
origin/masterin/tmp/eb107, ran it:1 passed. Then restoredmaster'sinstall.batunder the new test and re-ran:1 failed, with the failure quoting the offending line and pointing at the unterminated literal. So it is a regression test, not a restatement — the property it asserts was false before this change and is true after. - The replacement API is the right one, and better than what it replaces.
sysconfig.get_path('scripts', scheme='nt_user')returns the per-user Scripts directory directly. The original computed it by takingsite.getusersitepackages()and string-replacing\lib\site-packageswith\Scripts— which on Windows would not have matched even had it parsed, sincegetusersitepackages()there returns…\Python312\site-packageswith nolibcomponent. So the old line was broken twice over, and the fix removes the string surgery rather than repairing it. nt_useris a long-standing scheme name, available well below this project'srequires-python = ">=3.8"floor, so the fallback logic does not depend on a recent Python.- The extraction slice works on the real lines.
rfind('"')lands on the quote closing the Python command rather than on the one in"delims=", because the latter comes first — I confirmed the extracted text is exactlyimport sysconfig; print(sysconfig.get_path('scripts', scheme='nt_user')). It is fragile to a line that ends with any other double-quoted token, but no such line exists in this file today.
Architecture conformance
Conforms. §21 Tier 1 — Foundation (ebuild). install.bat is host-side installation tooling and tests/unit/ is the correct home for the guard per .ai/architect.md; nothing here is compiled into an image, so §5.1's dependency direction and eBuild's "never a runtime dependency" position are untouched.
The design requirement this serves is §25.2's Minimum Lovable Product list — "one-command environment diagnosis" — and §27's activation metrics, "eBuild installs, first simulation, first build". A Windows installer whose PATH step has never worked is a direct hit on the north-star metric in §39, external developer time-to-success: the very first command a Windows evaluator runs leaves ebuild off the PATH with no error. Worth stating because it makes a two-line change more important than its size, and because §22's support-tier language ("do not market all board descriptors as equivalent") has an obvious analogue for host platforms that the design does not currently draw.
No proposal appended. The gap here is repo-level — a script with no CI coverage and no evidence requirement — and it is already inside the scope of the existing proposal in .ai/autoreview/proposals/2026-09.md, "The evidence policy is silent on checks that verify nothing" (§28). Its §28.2 draft covers checks that cannot fail; this file's problem is the adjacent one, a file with no check at all, which §28's Implemented row ("code and functional tests") already addresses in principle. Adding a near-duplicate entry would dilute the record rather than sharpen it.
Proposed changes
- Extend the test to the batch shape as well as the Python (finding 1). A few lines in a file that already does the parsing, and it is the difference between guarding the bug that happened and guarding the file.
- Get the workflow runs approved and put real output behind the validation claim (finding 2).
- Swap the probe order (finding 3) and drop the BOM (finding 4). Both one-liners.
All four are independent. None of them is a reason to hold the fix itself, which is correct and repairs something that is broken on master right now.
Not checked
- Nothing was run on Windows, which is the only platform this change affects. I could not execute
install.bat, could not confirmSCRIPTS_DIRends up correct in either the venv or the per-user case, and could not confirm thatcmd.exeparses the new literal single quotes as I reason it does in finding 1. That reasoning is the weakest load-bearing claim in this review and it is the author who is positioned to settle it. sysconfig.get_path('scripts', scheme='nt_user')was not evaluated on Windows. On this Linux host the scheme resolves to a POSIX-shaped path, which tells me nothing about the value a Windows interpreter returns. That both probes return the right directories is taken from the API contract, not observed.- The full suite was not run, so "All existing tests pass" is uncorroborated beyond the one new file. On this host the suite has a pre-existing failure caused by a missing
ninjamodule, unrelated to this PR. - I did not check the rest of
install.bat. Only lines 55 and 58 contain embedded Python; the PATH-writing step at line 66 (reg query HKCU\Environment /v PATH) and everything around it I did not read, so whether the installer works onceSCRIPTS_DIRis correct is an open question this PR does not answer and I did not investigate. - No equivalent POSIX installer was compared. If
install.shexists and computes the same directory a different way, the two may now disagree; I did not look. mergeStateStatus: BLOCKED,mergeable: MERGEABLE,reviewDecision: REVIEW_REQUIRED. No merge attempted. Neither file is touched by any other PR in this batch, so no conflicts.- The local
ebuildcheckout is dirty and was skipped by the sync step, and sits on branchv90. I readorigin/masterthroughgit showandgit archive; the working tree was not touched and all patched trees are under/tmp.
Automated architecture review of f61353e852aa — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
srpatcha
reviewed
Sep 4, 2026
srpatcha
left a comment
srpatcha
left a comment
Member
There was a problem hiding this comment.
Review — ebuild#107 "fix Windows installer Scripts path detection"
head: c93910c author: Dhananjay2799 ci: none reported
Verdict: The path-detection fix is correct and the regression test is a good one — I
confirmed it fails on the pre-fix line. But repairing this detection makes a
PATH-destroying block ten lines below it reachable for the first time, and that block
overwrites the user's persistent HKCU PATH. That has to be fixed in the same PR.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Critical | install.bat:66-69 |
USER_PATH is assigned at line 66 and read at lines 67 and 69 — all three inside the if exist (...) block opened at line 61. The script runs setlocal (line 9) without EnableDelayedExpansion, so cmd.exe substitutes %USER_PATH% when it parses the whole block, before line 66 executes. USER_PATH is set nowhere earlier in the file, so both reads expand to empty. Line 69 therefore runs setx PATH ";%SCRIPTS_DIR%", which writes that string to the user's persistent HKCU\Environment PATH, discarding everything previously in it. >nul 2>&1 hides it, so the user is told [OK] Added ... to user PATH while their PATH is being destroyed. This PR is what makes the block reachable — see the note below. |
Add EnableDelayedExpansion to line 9 and use !USER_PATH! at lines 67 and 69; or hoist the reg query above line 61 so the value exists before the block is parsed. Either way add a guard: if USER_PATH is empty, do not call setx PATH at all — write only %SCRIPTS_DIR% or skip and warn. An empty USER_PATH is indistinguishable from "read failed", and the destructive branch must not be the default. |
| 2 | Medium | install.bat:40-45, 78-81, 99 |
The installer cannot report failure. Lines 40 and 43 send pip's stderr to nul and neither result is tested, so line 45 prints [OK] Python package installed even when both pip invocations failed. The else at line 78 prints [WARN] Could not find ebuild.exe, falls through to :verify, and the script ends at line 99 with no exit /b, so every one of these paths exits 0. .ai/tooling.md (CLI conventions): "Exit non-zero on failure, always." A verification whose result is discarded is a finding per .ai/reviewer.md. |
Test %ERRORLEVEL% after the second pip attempt and exit /b 1 with pip's actual output shown; exit /b 1 from the line 78 branch and from the "installed but not yet on PATH" branch at line 94. |
| 3 | Low | tests/unit/test_windows_installer.py:42-49, :77 |
_installer_python_commands() selects every line containing -c ", not every line the installer uses to set SCRIPTS_DIR. Any future -c " in install.bat that is not a for /f ... SCRIPTS_DIR line makes test_embedded_python_commands_are_well_formed fail against a correct installer, and assert len(lines) == 2 in the cmd.exe test breaks on any third one. |
Select with BATCH_COMMAND_PATTERN itself rather than the -c " substring, and assert the list is non-empty instead of exactly 2. |
| 4 | Low | PR body, "Testing" / "Additional Notes" | "Validated on Windows with Python 3.14.6 across active virtual environments and per-user installation paths" carries no output. The checklist simultaneously leaves "Unit tests pass" unchecked and checks "All existing tests pass", which cannot both be current. Per the review brief, an unsupported validation claim is itself the finding. | Paste the Windows run, or drop the claim and say what was and was not exercised. |
Architecture conformance
Conforms. ebuild is Tier 1 — Foundation (§21), and its own Windows installer belongs in
it; nothing here points up a tier (§5.1). §9.2 requires "actionable diagnostics with
remediation guidance", which the line 79-80 warning does provide — the problem is finding
2, that the guidance is not accompanied by a non-zero exit.
Proposed changes
Smallest sequence that keeps things working:
- Fix finding 1 before this merges. As it stands, merging improves detection and thereby
turns on the destructive branch. Fixing detection and fixingsetxbelong in the same
change because it is this PR that couples them. - Extend
test_embedded_python_commands_execute_in_cmdto cover the PATH block, not just
the twofor /flines — set a knownUSER_PATH, run the block against a
setxstub, and assert the composed value still contains the original PATH. That is
the assertion that would have caught this, and it runs for real:ci.yml:20includes
windows-2022in the matrix. - Findings 2 and 3 can follow in the same PR; both are a few lines.
What I verified
Ran on the PR head, extracted to a scratch directory (repository untouched):
- Both new one-liners parse and compile:
import sysconfig; print(sysconfig.get_path('scripts'))and thescheme='nt_user'
variant.nt_useris present insysconfig.get_scheme_names()on the Python here (3.14). - The new test genuinely catches the bug it claims to. Applying
BATCH_COMMAND_PATTERNto the pre-fix line matches, andcompile()on the captured
command raisesSyntaxError: unterminated string literal. The test fails on the old
file and passes on the new one. - Both pre-fix commands were invalid Python, not just the first: the old fallback
os.path.join(os.path.dirname(sys.executable), chr(39)Scripts')also raises
SyntaxError: unterminated string literal. So before this PRSCRIPTS_DIRwas always
empty,if exist "%SCRIPTS_DIR%\ebuild.exe"at line 61 was always false, and thesetx
block at 66-69 never executed. That is the basis for finding 1 being this PR's problem
rather than a pre-existing one to defer.
The substitution the new code makes is also a real improvement in coverage, not just a
syntax repair: the default scheme resolves a venv's Scripts and a system prefix's
Scripts, and nt_user resolves the pip install --user location that pip falls back to
on Windows when the prefix is not writable. The old pair, had it parsed, would have missed
the last case.
Not checked
- I could not execute
cmd.exe— this host is Linux. Finding 1 rests on documented
cmd.exeparse-time%VAR%expansion inside a parenthesised block and on reading the
file; thesetxoverwrite is Inferred, not Verified. It needs one run on Windows to
confirm, and that run should be done with a saved copy ofHKCU\Environment\PATH.
Because I cannot run it, I have not opened a fix PR: the policy here is no fix without a
verification I can execute. pytestis not installed on this host andpython3 -m venvis unavailable, so I
reproduced the two assertions oftest_embedded_python_commands_are_well_formedby hand
rather than running the file under pytest. I did not run the rest oftests/.- CI:
checks.txtis empty. No check runs were reported for this head at bundle time.
I did not establish why.ci.ymldoes define awindows-2022leg, so the cmd.exe test
is not dead — but I have no evidence it ran for this commit. - Not examined: whether
pip install -eon Windows placesebuild.exein the location
the primary command returns in every supported install mode (Store Python, thepy
launcher,--targetinstalls). Only the three modes named above were reasoned about.
Automated architecture review of c93910ca6fc4 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
Author
|
Thanks for the detailed review. I’ve pushed a follow-up commit (
I also updated the PR description with the actual Windows validation output and clarified that the full repository suite is currently blocked by an unrelated pre-existing Ready for another review. Thank you. |
Author
|
Hi,
I’m following up regarding the technical assessment for the Software and
Systems Engineer position.
I have completed the eBuild bug-fix assignment and submitted the
implementation as PR #107. The review confirmed that the Windows installer
issue was reproducible on master and that the sysconfig-based fix correctly
addresses the problem.
I also addressed the follow-up review feedback by:
- Strengthening the regression test to validate the full Windows FOR /F
batch command structure
- Adding actual cmd.exe execution coverage on Windows
- Updating the Scripts-directory probe order
- Removing the UTF-8 BOM from the test file
- Adding the Windows validation results and test evidence to the PR
description
The updated submission is available here:
#107
I remain very interested in the AI Software and Systems Engineer
opportunity. Please let me know if there are any additional changes or
information you would like from me, and I would appreciate any guidance on
the next steps in the interview process.
Thank you for your time and consideration.
Best regards,
Dhananjay Patel
…On Thu, Sep 3, 2026 at 9:11 PM Srikanth Patchava ***@***.***> wrote:
***@***.**** commented on this pull request.
Review — ebuild#107 "fix Windows installer Scripts path detection"
head: c93910c
<c93910c>
author: Dhananjay2799 ci: none reported
*Verdict:* The path-detection fix is correct and the regression test is a
good one — I
confirmed it fails on the pre-fix line. But repairing this detection makes
a
PATH-destroying block ten lines below it reachable for the first time, and
that block
overwrites the user's persistent HKCU PATH. That has to be fixed in the
same PR.
Findings
# Severity File:line Finding Recommended fix
1 Critical install.bat:66-69 USER_PATH is assigned at line 66 and read at
lines 67 and 69 — all three inside the if exist (...) block opened at
line 61. The script runs setlocal (line 9) *without*
EnableDelayedExpansion, so cmd.exe substitutes %USER_PATH% when it parses
the whole block, before line 66 executes. USER_PATH is set nowhere
earlier in the file, so both reads expand to empty. Line 69 therefore runs setx
PATH ";%SCRIPTS_DIR%", which *writes that string to the user's persistent
HKCU\Environment PATH, discarding everything previously in it*. >nul 2>&1
hides it, so the user is told [OK] Added ... to user PATH while their
PATH is being destroyed. *This PR is what makes the block reachable* —
see the note below. Add EnableDelayedExpansion to line 9 and use
!USER_PATH! at lines 67 and 69; or hoist the reg query above line 61 so
the value exists before the block is parsed. Either way add a guard: if
USER_PATH is empty, do not call setx PATH at all — write only
%SCRIPTS_DIR% or skip and warn. An empty USER_PATH is indistinguishable
from "read failed", and the destructive branch must not be the default.
2 Medium install.bat:40-45, 78-81, 99 The installer cannot report
failure. Lines 40 and 43 send pip's stderr to nul and neither result is
tested, so line 45 prints [OK] Python package installed even when both
pip invocations failed. The else at line 78 prints [WARN] Could not find
ebuild.exe, falls through to :verify, and the script ends at line 99 with
no exit /b, so every one of these paths exits 0. .ai/tooling.md (CLI
conventions): "Exit non-zero on failure, always." A verification whose
result is discarded is a finding per .ai/reviewer.md. Test %ERRORLEVEL%
after the second pip attempt and exit /b 1 with pip's actual output
shown; exit /b 1 from the line 78 branch and from the "installed but not
yet on PATH" branch at line 94.
3 Low tests/unit/test_windows_installer.py:42-49, :77
_installer_python_commands() selects every line containing -c ", not
every line the installer uses to set SCRIPTS_DIR. Any future -c " in
install.bat that is not a for /f ... SCRIPTS_DIR line makes
test_embedded_python_commands_are_well_formed fail against a correct
installer, and assert len(lines) == 2 in the cmd.exe test breaks on any
third one. Select with BATCH_COMMAND_PATTERN itself rather than the -c "
substring, and assert the list is non-empty instead of exactly 2.
4 Low PR body, "Testing" / "Additional Notes" "Validated on Windows with
Python 3.14.6 across active virtual environments and per-user installation
paths" carries no output. The checklist simultaneously leaves "Unit tests
pass" unchecked and checks "All existing tests pass", which cannot both be
current. Per the review brief, an unsupported validation claim is itself
the finding. Paste the Windows run, or drop the claim and say what was
and was not exercised. Architecture conformance
Conforms. ebuild is Tier 1 — Foundation (§21), and its own Windows
installer belongs in
it; nothing here points up a tier (§5.1). §9.2 requires "actionable
diagnostics with
remediation guidance", which the line 79-80 warning does provide — the
problem is finding
2, that the guidance is not accompanied by a non-zero exit.
Proposed changes
Smallest sequence that keeps things working:
1. Fix finding 1 before this merges. As it stands, merging improves
detection and thereby
turns on the destructive branch. Fixing detection and fixing setx
belong in the same
change because it is this PR that couples them.
2. Extend test_embedded_python_commands_execute_in_cmd to cover the
PATH block, not just
the two for /f lines — set a known USER_PATH, run the block against a
setx stub, and assert the composed value still contains the original
PATH. That is
the assertion that would have caught this, and it runs for real:
ci.yml:20 includes
windows-2022 in the matrix.
3. Findings 2 and 3 can follow in the same PR; both are a few lines.
What I verified
Ran on the PR head, extracted to a scratch directory (repository
untouched):
- Both new one-liners parse and compile:
import sysconfig; print(sysconfig.get_path('scripts')) and the
scheme='nt_user'
variant. nt_user is present in sysconfig.get_scheme_names() on the
Python here (3.14).
- The new test genuinely catches the bug it claims to. Applying
BATCH_COMMAND_PATTERN to the pre-fix line matches, and compile() on
the captured
command raises SyntaxError: unterminated string literal. The test
fails on the old
file and passes on the new one.
- *Both* pre-fix commands were invalid Python, not just the first: the
old fallback
os.path.join(os.path.dirname(sys.executable), chr(39)Scripts') also
raises
SyntaxError: unterminated string literal. So before this PR SCRIPTS_DIR
was always
empty, if exist "%SCRIPTS_DIR%\ebuild.exe" at line 61 was always
false, and the setx
block at 66-69 never executed. That is the basis for finding 1 being
this PR's problem
rather than a pre-existing one to defer.
The substitution the new code makes is also a real improvement in
coverage, not just a
syntax repair: the default scheme resolves a venv's Scripts and a system
prefix's
Scripts, and nt_user resolves the pip install --user location that pip
falls back to
on Windows when the prefix is not writable. The old pair, had it parsed,
would have missed
the last case.
Not checked
- *I could not execute cmd.exe* — this host is Linux. Finding 1 rests
on documented
cmd.exe parse-time %VAR% expansion inside a parenthesised block and on
reading the
file; the setx overwrite is *Inferred, not Verified*. It needs one run
on Windows to
confirm, and that run should be done with a saved copy of
HKCU\Environment\PATH.
Because I cannot run it, I have not opened a fix PR: the policy here
is no fix without a
verification I can execute.
- pytest is not installed on this host and python3 -m venv is
unavailable, so I
reproduced the two assertions of
test_embedded_python_commands_are_well_formed by hand
rather than running the file under pytest. I did *not* run the rest of
tests/.
- *CI: checks.txt is empty.* No check runs were reported for this head
at bundle time.
I did not establish why. ci.yml does define a windows-2022 leg, so the
cmd.exe test
is not dead — but I have no evidence it ran for this commit.
- Not examined: whether pip install -e on Windows places ebuild.exe in
the location
the primary command returns in every supported install mode (Store
Python, the py
launcher, --target installs). Only the three modes named above were
reasoned about.
------------------------------
*Automated architecture review of c93910c — scheduled, model
claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory
only: this reviewer never approves, requests changes, or merges. Reply here
to discuss or push back — a wrong finding is a bug worth reporting.*
—
Reply to this email directly, view it on GitHub
<#107?email_source=notifications&email_token=AI5BDWY4FLF5SNA6TVILKAL5NIJD5A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQHAZDKNBVGMYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5108254530>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI5BDW4RUYGFG6SBG7ZA5QL5NIJD5AVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJQGEZDKNZYGE5US43TOVSTWNJTGI2TSNRXGQ4DNILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AI5BDW5PFBLFXIQINIWELJT5NIJD5A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQHAZDKNBVGMYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AI5BDWYVCDZZMWJG3B4NCTT5NIJD5A5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMJQHAZDKNBVGMYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
<embeddedos-org/ebuild/pull/107/review/5108254530 <(510)%20825-4530>@
github.com>
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes broken Windows
Scriptsdirectory detection ininstall.bat.The original embedded Python commands were syntactically invalid, causing the
FOR /Fprobes to produce no output and leavingSCRIPTS_DIRunset.This PR replaces the manual path construction with Python's standard
sysconfigAPI, checks the active Python environment first, and falls back to the per-user Scripts directory when necessary.Regression coverage now validates both the embedded Python and the surrounding Windows batch syntax, including execution through the real
cmd.exe.Type of Change
Changes
install.batwithsysconfig.get_path().nt_userScripts directory as the fallback.FOR /Fbatch command structure.cmd.exe.tests/unit/test_windows_installer.py.Testing
ctest --test-dir build --output-on-failure)Focused regression tests
Run on Windows with Python 3.14.6:
Lint validation
Result: no issues reported.
Windows
cmd.exevalidationThe installer detection logic was also exercised through the actual Windows command interpreter.
The active virtual-environment Scripts directory was resolved successfully and
ebuild.exewas found.Full repository test suite
I also attempted to run the complete repository pytest suite.
Collection is currently blocked by a pre-existing syntax error in the unrelated file:
That file is unchanged by this PR, so I kept this fix scoped to the Windows installer issue.
Pre-Submission Checklist
-Wall -Wextra -Werrorfor C)<type>(<scope>): <description>conventionRelated Issues
N/A
Screenshots / Logs
Additional Notes
Validated on Windows with Python 3.14.6 using both an active virtual environment and the per-user Python Scripts path.
The implementation remains intentionally scoped to Windows Scripts-directory detection. No unrelated source files were modified.