fix: apply review fixes on craftos-just-plan
This commit is contained in:
parent
23d546c3a9
commit
b190c9cc09
@ -41,11 +41,15 @@ Prefer read-only mounts by default. Use explicit caller-provided args for unusua
|
||||
|
||||
3. Add `check-luacheck` to `Justfile` so install checks every host-side CLI requirement explicitly.
|
||||
|
||||
`check-jq` and `check-luacheck` are tool-presence checks only (verify the binary is on `$PATH`), mirroring the shape of the existing `check-craftos`. They do not run lint.
|
||||
|
||||
4. Add `check-install: check-craftos check-jq check-luacheck`.
|
||||
|
||||
5. Change `install` from `install-git-hooks check-craftos` to `install-git-hooks check-install`.
|
||||
|
||||
6. Change `check` to depend on `check-luacheck` and keep `luacheck .` as the command.
|
||||
6. Change `check` to depend on `check-luacheck` and keep `luacheck .` as the command. Because `check-luacheck` only verifies presence (step 3), no lint runs twice.
|
||||
|
||||
Leave `ci: check-craftos check` unchanged. `ci` deliberately does not pull in `check-jq`: the pre-commit path does not use `jq`, so adding it would inflate the hook's required tooling for no benefit. The two dep chains (`install → check-install`, `ci → check-craftos + check → check-luacheck`) are intentional, not an oversight to unify later.
|
||||
|
||||
7. Add `craftos *args: check-install`.
|
||||
|
||||
@ -55,24 +59,34 @@ Recommended behavior:
|
||||
- Keep the existing macOS `--rom /Applications/CraftOS-PC.app/Contents/Resources` workaround.
|
||||
- Add `--mount-ro "/trapos={{justfile_directory()}}"`.
|
||||
- Use `jq` over `manifest.json` to derive unique top-level directories and add one `--mount-ro "/<dir>={{justfile_directory()}}/<dir>"` per directory.
|
||||
- Forward user args after the recipe defaults.
|
||||
- Forward user args after the recipe defaults. `just` forwards flags to variadic recipes normally, so callers should use commands like `just craftos --headless` and `just craftos --exec 'print("__READY__"); os.shutdown()'` without an extra `--` separator.
|
||||
|
||||
Build the recipe-generated argv with quoted arguments and invoke `craftos` with forwarded user args as `"$@"` so paths and `--exec` code containing spaces survive. Do not use unquoted `$mount_args` word splitting, and do not use `{{args}}` for forwarding because it does not preserve shell quoting.
|
||||
|
||||
Sketch:
|
||||
|
||||
```just
|
||||
# Launch CraftOS-PC with repo-local data and read-only repo mounts. Pass args through to `craftos`.
|
||||
# Launch CraftOS-PC with repo-local data and read-only repo mounts.
|
||||
# Pass args through to `craftos`, for example:
|
||||
# just craftos --headless --exec 'print("__READY__"); os.shutdown()'
|
||||
[positional-arguments]
|
||||
craftos *args: check-install
|
||||
@rom_arg=""; \
|
||||
if [ "$(uname -s)" = "Darwin" ]; then \
|
||||
rom_arg="--rom /Applications/CraftOS-PC.app/Contents/Resources"; \
|
||||
fi; \
|
||||
mount_args="--mount-ro /trapos={{justfile_directory()}}"; \
|
||||
for dir in $(jq -r '.files[] | split("/")[0]' manifest.json | sort -u); do \
|
||||
mount_args="$mount_args --mount-ro /$dir={{justfile_directory()}}/$dir"; \
|
||||
done; \
|
||||
exec craftos --directory "{{justfile_directory()}}/.craftos" $rom_arg $mount_args {{args}}
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
repo='{{justfile_directory()}}'
|
||||
argv=(--directory "$repo/.craftos")
|
||||
if [ "$(uname -s)" = "Darwin" ]; then
|
||||
argv+=(--rom /Applications/CraftOS-PC.app/Contents/Resources)
|
||||
fi
|
||||
argv+=(--mount-ro "/trapos=$repo")
|
||||
while IFS= read -r dir; do
|
||||
argv+=(--mount-ro "/$dir=$repo/$dir")
|
||||
done < <(jq -r '.files[] | split("/")[0]' manifest.json | sort -u)
|
||||
exec craftos "${argv[@]}" "$@"
|
||||
```
|
||||
|
||||
Verify the shebang recipe receives variadic args as `"$@"` before committing. If it does not, use a linewise `[positional-arguments]` recipe that delegates to `bash -c '...' craftos "$@"`; do not fall back to `{{args}}`.
|
||||
|
||||
8. Add `repl` as an interactive human-only wrapper.
|
||||
|
||||
```just
|
||||
@ -118,6 +132,18 @@ craftos -d <temp-data> \
|
||||
--exec 'print(fs.exists("/apis/net.lua")); print(fs.exists("/trapos/manifest.json")); os.shutdown()'
|
||||
```
|
||||
|
||||
4. macOS `-d` + `--rom` combo (the recipe's actual shape):
|
||||
|
||||
```sh
|
||||
craftos -d <temp-data> \
|
||||
--rom /Applications/CraftOS-PC.app/Contents/Resources \
|
||||
--mount-ro /trapos=<repo> \
|
||||
--headless \
|
||||
--exec 'print(fs.exists("/trapos/manifest.json")); os.shutdown()'
|
||||
```
|
||||
|
||||
This must succeed before committing the macOS branch of the recipe — `-d` changes data-root resolution, and the existing test recipe only proves `--rom` works without `-d`.
|
||||
|
||||
Choose mounts unless `-C` demonstrably gives the desired repo-root ComputerCraft layout without repository pollution.
|
||||
|
||||
## Verification
|
||||
|
||||
Loading…
Reference in New Issue
Block a user