no-bug: A few quality-of-life tweaks, take 2#13637
Open
iskunk wants to merge 1 commit into
Open
Conversation
dosubot
Bot
added
size:M
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make npm run import more cross-platform (notably on Windows) by avoiding direct cargo invocation from package.json, and by making path inputs configurable for the supporting import scripts/tools.
Changes:
- Updated
npm run ffprefsto invoke Cargo via a small Python wrapper that honors theCARGOenvironment variable. - Updated
ffprefsto accept explicitprefsandenginepaths (instead of relying on a shared root +chdir). - Added optional CLI overrides for dump folder paths in
update_service_dumps.py.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/ffprefs/src/main.rs | Switches ffprefs from chdir-based root handling to explicit prefs/engine path arguments. |
| scripts/update_service_dumps.py | Adds optional argv-based overrides for dumps folder locations. |
| scripts/run_cargo.py | Introduces a Python wrapper to execute Cargo, honoring $CARGO when set. |
| package.json | Uses run_cargo.py for ffprefs and passes prefs/engine args using the new convention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
340
to
+356
| fn main() { | ||
| let args: Vec<String> = env::args().collect(); | ||
| let root_path = if args.len() > 1 { | ||
| let prefs_path = if args.len() > 1 { | ||
| PathBuf::from(&args[1]) | ||
| } else { | ||
| env::current_dir().expect("Failed to get current directory") | ||
| PathBuf::from("prefs") | ||
| }; | ||
| let engine_path = if args.len() > 2 { | ||
| PathBuf::from(&args[2]) | ||
| } else { | ||
| PathBuf::from("engine") | ||
| }; | ||
| env::set_current_dir(&root_path).expect("Failed to change directory"); | ||
|
|
||
| prepare_zen_prefs(); | ||
| let mut preferences = load_preferences(); | ||
| prepare_zen_prefs(&engine_path); | ||
| let mut preferences = load_preferences(&prefs_path); | ||
| expand_pref_values(&mut preferences); | ||
| write_preferences(&preferences); | ||
| write_preferences(&engine_path, &preferences); |
Contributor
Author
There was a problem hiding this comment.
Zen build tooling already works with $CWD/.surfer/.
Comment on lines
+14
to
+15
| except Exception: | ||
| print('Exec failed', file=sys.stderr) |
Contributor
Author
There was a problem hiding this comment.
I've pushed an update that makes the error message more informative.
Comment on lines
+64
to
66
| if len(sys.argv) == 3: | ||
| _, DUMPS_FOLDER, ENGINE_DUMPS_FOLDER = sys.argv | ||
| main() |
Contributor
Author
There was a problem hiding this comment.
update_service_dumps.py is not a general-purpose utility that needs extensive input validation and usage information. My goal here was a minimal change that allows the locations to be specified externally.
package.json: Invoke cargo(1) through a Python script that honors the CARGO environment variable if set, and pass arguments to ffprefs using the new convention (see below) run_cargo.py: Script to invoke cargo(1), as a workaround for reading an environment variable in a package.json script in a cross-platform manner update_service_dumps.py: Allow specifying DUMPS_FOLDER and ENGINE_DUMPS_FOLDER on the command line ffprefs/src/main.rs: Allow specifying the prefs and engine dirs on the command line instead of hard-coding their locations relative to a common root dir
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
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.
This should support
npm run importon Windows. I looked into run-script-os, and a couple other approaches, but then figured it would be simpler to just call a small Python script that looks up the environment variable and callscargo/$CARGOas appropriate.Please give this a try, and let me know if you see any regressions.
package.json: Invokecargo(1)through a Python script that honors theCARGOenvironment variable if set, and pass arguments toffprefsusing the new convention (see below)run_cargo.py: Script to invokecargo(1), as a workaround for reading an environment variable in apackage.jsonscript in a cross-platform mannerupdate_service_dumps.py: Allow specifyingDUMPS_FOLDERandENGINE_DUMPS_FOLDERon the command lineffprefs/src/main.rs: Allow specifying theprefsandenginedirs on the command line instead of hard-coding their locations relative to a common root dir