Dev#5
Merged
Merged
Conversation
Veuillez saisir le message de validation pour vos modifications. Les lignes commençant par '' seront ignorées, et un message vide abandonne la validation. Sur la branche dev Votre branche est à jour avec 'origin/dev'. Modifications qui seront validées : modifié : install_ventoy.py modifié : windows/flash.py modifié : windows/tweaks.py
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors several Windows flashing and tweaking routines to rely on shell commands (via run_cmd/subprocess) for filesystem and dependency operations, replacing direct Python shutil/os helpers, and updates dependency detection/installation logic to use explicit Flow diagram for Windows install tweaks using external commandsgraph TD
A_start["Start tweak function (win_hardware_bypass or win_local_acc)"] --> B_prepare_cmds[Prepare registry commands string]
B_prepare_cmds --> C_log_start[Log start message]
C_log_start --> D_try_block[Enter try block]
D_try_block --> E_mkdir[Run subprocess mkdir /media/tempwinmnt]
E_mkdir --> F_mount_wim[Run wimmountrw boot.wim index 2 /media/tempwinmnt]
F_mount_wim --> G_chntpw[Run chntpw e SYSTEM or SOFTWARE hive with commands]
G_chntpw --> H_unmount_wim[Run wimunmount /media/tempwinmnt --commit]
H_unmount_wim --> I_cleanup[Run subprocess rm -rf /media/tempwinmnt]
I_cleanup --> J_log_success[Log success message]
J_log_success --> K_end_success[End function]
D_try_block --> L_except[On subprocess.CalledProcessError]
L_except --> M_log_error[Log error with stderr]
M_log_error --> N_cleanup_fail[Optionally leave or retry cleanup]
N_cleanup_fail --> O_end_error[End function with error path]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The replacements of Python file operations (os.makedirs, shutil.copy[tree], shutil.rmtree, os.sync) with shell commands (mkdir/cp/rm/sync via subprocess) reduce portability and error transparency, and in several places introduce unnecessary sudo usage; consider keeping the Python APIs for filesystem work and only using external commands where strictly required (e.g., tools that don’t have Python equivalents).
- In _copy_efi_boot_files, changing the per-item, path-aware copy logic to a single
cp -r ... mount_eficall alters the destination layout (dropping the explicit EFI/ and boot/ subdirectories) and directory-creation semantics (dirs_exist_ok, file vs directory targets); please double-check that the new cp commands reproduce the intended directory structure and overwrite behavior. - The new uses of
mkdirandrm -rf(e.g., in win_hardware_bypass and win_local_acc) no longer replicate the priorexist_ok=True/ignore_errors=Truebehavior and will now raise on existing directories or some error conditions; if the original idempotent behavior is desired, add flags likemkdir -pand adjust error handling to match.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The replacements of Python file operations (os.makedirs, shutil.copy[tree], shutil.rmtree, os.sync) with shell commands (mkdir/cp/rm/sync via subprocess) reduce portability and error transparency, and in several places introduce unnecessary sudo usage; consider keeping the Python APIs for filesystem work and only using external commands where strictly required (e.g., tools that don’t have Python equivalents).
- In _copy_efi_boot_files, changing the per-item, path-aware copy logic to a single `cp -r ... mount_efi` call alters the destination layout (dropping the explicit EFI/ and boot/ subdirectories) and directory-creation semantics (dirs_exist_ok, file vs directory targets); please double-check that the new cp commands reproduce the intended directory structure and overwrite behavior.
- The new uses of `mkdir` and `rm -rf` (e.g., in win_hardware_bypass and win_local_acc) no longer replicate the prior `exist_ok=True` / `ignore_errors=True` behavior and will now raise on existing directories or some error conditions; if the original idempotent behavior is desired, add flags like `mkdir -p` and adjust error handling to match.
## Individual Comments
### Comment 1
<location path="src/lufus/writing/windows/flash.py" line_range="274-281" />
<code_context>
+ run_cmd(["sudo", "cp", "-r"] + [os.path.join(efi_src, i) for i in efi_items] + [mount_efi])
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `cp -r … mount_efi` calls change the target layout compared to the previous `EFI/` and `boot/` subdirectories.
With this change, `efi_src` and `boot_src` contents end up directly under `mount_efi` instead of under `mount_efi/EFI` and `mount_efi/boot`, which changes the on-disk layout and can break UEFI boot.
If the original directory structure is required, the destination should include the subdirectory (e.g. `... + [os.path.join(mount_efi, "EFI")]` and similarly for `boot`), and those directories should be ensured to exist or created by `cp` as appropriate.
</issue_to_address>
### Comment 2
<location path="src/lufus/writing/windows/flash.py" line_range="158-161" />
<code_context>
"""Find mkfs.ntfs/mkntfs, installing ntfs-3g if needed. Returns command name or None."""
for candidate in ["mkfs.ntfs", "mkntfs"]:
- if shutil.which(candidate):
+ if subprocess.run(["which", candidate], capture_output=True).returncode == 0:
return candidate
</code_context>
<issue_to_address>
**suggestion:** Replacing `shutil.which` with `subprocess.run(["which", …])` reduces portability and adds overhead.
`shutil.which` is cross‑platform and doesn’t rely on an external `which` binary being in `PATH`, while `subprocess.run(["which", …])` does and also adds process‑spawn overhead inside the loop.
Unless there’s a strong need for `which`’s exact semantics, consider reverting to `shutil.which` here and reserve `run_cmd`/`sudo` for actually invoking the tools or package managers once found.
Suggested implementation:
```python
def _find_ntfs_tool(status_cb=None) -> str | None:
"""Find mkfs.ntfs/mkntfs, installing ntfs-3g if needed. Returns command name or None."""
for candidate in ["mkfs.ntfs", "mkntfs"]:
if shutil.which(candidate):
return candidate
```
1. Ensure `import shutil` is present at the top of `src/lufus/writing/windows/flash.py`. If `subprocess` is no longer used anywhere in this file, you can safely remove `import subprocess` to avoid an unused import.
2. No changes are required to `run_cmd`/`sudo` usage; keep those for actually invoking the tools or package managers once a candidate is found.
</issue_to_address>
### Comment 3
<location path="src/lufus/writing/windows/tweaks.py" line_range="57" />
<code_context>
log.info("win_hardware_bypass: injecting registry keys into boot.wim at %s...", mount)
try:
- os.makedirs("/media/tempwinmnt", exist_ok=True)
+ subprocess.run(["mkdir", "/media/tempwinmnt"], check=True)
subprocess.run(["wimmountrw", f"{mount}/sources/boot.wim", "2", "/media/tempwinmnt"], check=True)
subprocess.run(
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `mkdir` without `-p` loses the previous idempotent behavior of `os.makedirs(..., exist_ok=True)`.
With the previous `os.makedirs` call, rerunning this code when `/media/tempwinmnt` already existed was fine. With `mkdir` and `check=True`, an existing directory now causes a `CalledProcessError` that fails the whole operation. To keep retries/idempotency, either pass `-p` to `mkdir` or special‑case the “already exists” error.
</issue_to_address>
### Comment 4
<location path="src/lufus/writing/windows/tweaks.py" line_range="66-68" />
<code_context>
)
subprocess.run(["wimunmount", "/media/tempwinmnt", "--commit"], check=True)
- shutil.rmtree("/media/tempwinmnt", ignore_errors=True)
+ subprocess.run(["rm", "-rf", "/media/tempwinmnt"], check=True)
log.info("win_hardware_bypass: registry keys injected successfully.")
except subprocess.CalledProcessError as e:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `rm -rf` with `check=True` changes semantics compared to `shutil.rmtree(..., ignore_errors=True)`.
Previously, cleanup errors were explicitly ignored; now any failure in `rm -rf` will raise and send control to the `CalledProcessError` handler. If cleanup is still meant to be best-effort, consider removing `check=True` here or catching and logging failures from this specific cleanup step instead of letting them affect control flow.
```suggestion
subprocess.run(["wimunmount", "/media/tempwinmnt", "--commit"], check=True)
try:
subprocess.run(["rm", "-rf", "/media/tempwinmnt"], check=True)
except subprocess.CalledProcessError as cleanup_error:
log.warning(
"win_hardware_bypass: failed to clean up /media/tempwinmnt: %s",
cleanup_error,
)
log.info("win_hardware_bypass: registry keys injected successfully.")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+274
to
+281
| run_cmd(["sudo", "cp", "-r"] + [os.path.join(efi_src, i) for i in efi_items] + [mount_efi]) | ||
| _status("Copied EFI/ tree to EFI partition") | ||
| else: | ||
| _status("WARNING: No EFI directory found in ISO - drive may not be UEFI bootable") | ||
|
|
||
| boot_src = _find_path_case_insensitive(iso_mount, "boot") | ||
| if boot_src: | ||
| for item in os.listdir(boot_src): | ||
| src_path = os.path.join(boot_src, item) | ||
| dst_path = os.path.join(mount_efi, "boot", item) | ||
| if os.path.isdir(src_path): | ||
| shutil.copytree(src_path, dst_path, dirs_exist_ok=True) | ||
| else: | ||
| os.makedirs(os.path.dirname(dst_path), exist_ok=True) | ||
| shutil.copy2(src_path, dst_path) | ||
| run_cmd(["sudo", "cp", "-r"] + [os.path.join(boot_src, i) for i in os.listdir(boot_src)] + [mount_efi]) |
There was a problem hiding this comment.
issue (bug_risk): The new cp -r … mount_efi calls change the target layout compared to the previous EFI/ and boot/ subdirectories.
With this change, efi_src and boot_src contents end up directly under mount_efi instead of under mount_efi/EFI and mount_efi/boot, which changes the on-disk layout and can break UEFI boot.
If the original directory structure is required, the destination should include the subdirectory (e.g. ... + [os.path.join(mount_efi, "EFI")] and similarly for boot), and those directories should be ensured to exist or created by cp as appropriate.
Comment on lines
+158
to
161
| if subprocess.run(["which", candidate], capture_output=True).returncode == 0: | ||
| return candidate | ||
|
|
||
| if status_cb: |
There was a problem hiding this comment.
suggestion: Replacing shutil.which with subprocess.run(["which", …]) reduces portability and adds overhead.
shutil.which is cross‑platform and doesn’t rely on an external which binary being in PATH, while subprocess.run(["which", …]) does and also adds process‑spawn overhead inside the loop.
Unless there’s a strong need for which’s exact semantics, consider reverting to shutil.which here and reserve run_cmd/sudo for actually invoking the tools or package managers once found.
Suggested implementation:
def _find_ntfs_tool(status_cb=None) -> str | None:
"""Find mkfs.ntfs/mkntfs, installing ntfs-3g if needed. Returns command name or None."""
for candidate in ["mkfs.ntfs", "mkntfs"]:
if shutil.which(candidate):
return candidate- Ensure
import shutilis present at the top ofsrc/lufus/writing/windows/flash.py. Ifsubprocessis no longer used anywhere in this file, you can safely removeimport subprocessto avoid an unused import. - No changes are required to
run_cmd/sudousage; keep those for actually invoking the tools or package managers once a candidate is found.
| log.info("win_hardware_bypass: injecting registry keys into boot.wim at %s...", mount) | ||
| try: | ||
| os.makedirs("/media/tempwinmnt", exist_ok=True) | ||
| subprocess.run(["mkdir", "/media/tempwinmnt"], check=True) |
There was a problem hiding this comment.
issue (bug_risk): Using mkdir without -p loses the previous idempotent behavior of os.makedirs(..., exist_ok=True).
With the previous os.makedirs call, rerunning this code when /media/tempwinmnt already existed was fine. With mkdir and check=True, an existing directory now causes a CalledProcessError that fails the whole operation. To keep retries/idempotency, either pass -p to mkdir or special‑case the “already exists” error.
Comment on lines
66
to
68
| subprocess.run(["wimunmount", "/media/tempwinmnt", "--commit"], check=True) | ||
| shutil.rmtree("/media/tempwinmnt", ignore_errors=True) | ||
| subprocess.run(["rm", "-rf", "/media/tempwinmnt"], check=True) | ||
| log.info("win_hardware_bypass: registry keys injected successfully.") |
There was a problem hiding this comment.
suggestion (bug_risk): rm -rf with check=True changes semantics compared to shutil.rmtree(..., ignore_errors=True).
Previously, cleanup errors were explicitly ignored; now any failure in rm -rf will raise and send control to the CalledProcessError handler. If cleanup is still meant to be best-effort, consider removing check=True here or catching and logging failures from this specific cleanup step instead of letting them affect control flow.
Suggested change
| subprocess.run(["wimunmount", "/media/tempwinmnt", "--commit"], check=True) | |
| shutil.rmtree("/media/tempwinmnt", ignore_errors=True) | |
| subprocess.run(["rm", "-rf", "/media/tempwinmnt"], check=True) | |
| log.info("win_hardware_bypass: registry keys injected successfully.") | |
| subprocess.run(["wimunmount", "/media/tempwinmnt", "--commit"], check=True) | |
| try: | |
| subprocess.run(["rm", "-rf", "/media/tempwinmnt"], check=True) | |
| except subprocess.CalledProcessError as cleanup_error: | |
| log.warning( | |
| "win_hardware_bypass: failed to clean up /media/tempwinmnt: %s", | |
| cleanup_error, | |
| ) | |
| log.info("win_hardware_bypass: registry keys injected successfully.") |
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
1 participant
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 by Sourcery
Standardize Windows flashing and tweak routines on shell-based commands and improve external tool detection for NTFS and wimlib.
Enhancements:
Chores: