Repository navigation
Conversation
Copy Manifest-v<major>.toml.default, instantiate the workspace and precompile with the Julia on the PATH. No test run, no resolve, no juliaup add/default, no global Revise, no ~/.bashrc alias. bin/update_default_manifest updates the manifest of the Julia on the PATH instead of switching the juliaup default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bin/julia_major prints the major.minor of the Julia on the PATH; bin/install and bin/update_default_manifest both call it instead of parsing `julia --version` two different ways. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 1 inline, 0 off the diff
Good
- Does what the card says: the
Pkg.test()calls,Pkg.resolve(), the resolve-from-scratch fallback,juliaup add/default, the Revise install and the~/.bashrcalias are all gone, matching TL;DR and 'What it does now' line for line - One
Pkg.instantiate()on the root project replaces the four per-subproject resolves; the card correctly says these are[workspace]members sharing the root manifest (checked against the CLAUDE.md edit and rootProject.toml) - Fails loudly: the shebang is
#!/bin/bash -eu, so a failing instantiate or update, or a badJULIAUP_CHANNELmaking$(bin/julia_major)fail, stops the script with a non-zero exit instead of quietly re-resolving - The five near-identical 25-line resolve/instantiate heredocs and the duplicated 1.11 and ≥1.12 branches are now a single precompile loop, so §2's duplication is removed rather than moved (−592/+79)
run_precompilefailure detection and the docs pyexpat carve-out are unchanged apart from wrapping to 92 columns, so the precompile checks still catch the same failuresbin/update_default_manifestno longer copiesexamples/Manifest-v1.11.toml(a file nothing writes) and no longerrm -rfs the shared~/.julia/compiled, which fixes real bugs, and the card names both- Each removal (disk check, macOS MPLBACKEND, registry step, warm-ups) is named in the card with a reason, and
run_juliakeeping its own version parse is justified:copy_bin()copies onlyrun_juliaandcreate_sys_image2, confirmed in src/KiteModels.jl:1054 test/test_installationstill works:./install --updateon a fresh clone now seeds the live manifest from.defaultand runsPkg.update(), which adds General itself in a fresh depot- Verification covers the reported failure, exit codes, byte-identical manifest, and no changes to
~/.bashrcor the juliaup default; the one failed ci-local run is traced to #314, already on main
Not good
bin/install:140— The Revise hint is printed after the green 'Installation complete!' banner and exits 0, yetbin/run_julia(the next step that same banner points to) errors without Revise. Fresh users following the README will likely miss the hint and hit a broken first launch. Print it before the banner, or name the requirement in README/index.md.bin/julia_majorprints major.minor (e.g. 1.12), so the name says less than the output;julia_minor_versionorjulia_channelwould say what it returns- README.md:138 and docs/src/index.md:103 still describe a flow where
./installsets everything up, but a fresh user without global Revise now has./bin/run_juliafail onusing Revise, and neither page mentionsJULIAUP_CHANNEL; the card's release checklist asks for README/index to be reconciled bin/update_default_manifestcallsbin/julia_majora second time afterbin/install --updatehas already worked out the version; harmless, but two lookups can disagree if the environment changes between them- Removing the registry update means a user with an outdated General registry may now hit a hard instantiate failure; the card names this risk, but the script's error output gives no hint to run
Pkg.Registry.update()
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| fi | ||
| fi | ||
| unset _config_file | ||
| if ! julia --startup-file=no -e 'exit(Base.find_package("Revise") === nothing)'; then |
There was a problem hiding this comment.
MINOR: The Revise hint is printed after the green 'Installation complete!' banner and exits 0, yet bin/run_julia (the next step that same banner points to) errors without Revise. Fresh users following the README will likely miss the hint and hit a broken first launch. Print it before the banner, or name the requirement in README/index.md.
There was a problem hiding this comment.
Fair: bin/run_julia:36 does 'using Revise', so the hint belongs before the banner. The branch is on hold until @ufechner7 answers the numbered list on #323 (point 3), so it goes into that redo rather than a push now.
|
Local full suite: FAIL (1 min, Julia 1.13.0, one cell of the matrix) |
|
All four are fair and go into the redo after the per-point answers on #323, not a push now (I promised no pushes until then): rename bin/julia_major to bin/julia_channel since it prints major.minor; point README/index.md at global Revise and JULIAUP_CHANNEL; have update_default_manifest read the version once; print a Pkg.Registry.update() hint when instantiate fails. |
|
No |
TL;DR
bin/installnow copiesManifest-v<major>.toml.default, instantiates the workspace and precompiles it with the Julia on the PATH, and does nothing else. It used to runPkg.test()too, so a single erroring test (#314 on Julia 1.13) made the install exit 1 even though instantiate and precompile had worked. The box then treated the worktree as uninstallable.What was wrong
Apart from the test run, the script changed things outside the repo. It ran
juliaup addandjuliaup default, added Revise to the global environment and appendedalias jl=...to~/.bashrc(or~/.zshrc). Even with-yit chose the Julia version from thejuliaupdefault and then installed and switched to it. It also calledPkg.resolve()after copying the.default, which can move what the manifest pins. When that failed it deleted the manifest and resolved from scratch, so the pins were lost without any error. The four sub-projects were resolved one by one against manifests the script created, but they are[workspace]members and share the root manifest, so onePkg.instantiate()covers all of them.What it does now
juliaas found on the PATH. Another version is chosen withJULIAUP_CHANNEL=1.12 ./bin/install, whichbin/run_juliaalso respects. An unsupported or missing channel exits 1.bin/julia_major(new, 4 lines) prints the version's major.minor, and bothbin/installandbin/update_default_manifestread it from there..defaultand runsPkg.instantiate(). It never resolves.--updaterunsPkg.update()on the live manifest (seeded from the.defaultif missing) and leaves the.defaultalone.-yis accepted and does nothing, because the script no longer asks anything. It stays: the box runs./bin/install -yon every worktree, and the ecosystem's install contract names the flag.examples,examples_3d,testanddocswith the existing failure detection, deletes a stale system image, and copies.JETLSConfig.toml.bin/run_juliastill doesusing Revise.Also removed, each on purpose:
juliaon the PATH and checks for that instead.MPLBACKEND=qtaggexport. It only applied to the processes the install itself started.bin/run_juliasets it for the sessions where plots actually open.Pkg.Registryadd/update step.Pkg.instantiate()adds General itself when no registry exists. An outdated registry is the risk named under Verification.using MakieControlPlots, DSP/using KiteViewerswarm-ups. They loaded packages the precompile loop had just built, and they ended in|| true, so they could not change the result.bin/update_default_manifest, the only caller of--update, had the same problems. It switched thejuliaupdefault, only knew 1.11 and 1.12, copiedexamples/Manifest-v1.11.toml(which the workspace never writes) andrm -rf'd the shared~/.julia/compiledcaches. Now it runsinstall --updateand copies the live manifest onto the.defaultfor the Julia on the PATH. The install line inCLAUDE.mdnow says the same.Pushback
Dropping the version menu means someone at a terminal no longer gets asked which Julia to use. They set
JULIAUP_CHANNELinstead, which-hexplains. A menu that doesn't change thejuliaupdefault would install for one version whilerun_juliastarts another.Verification
./bin/install -ylog ended inSome tests did not pass: 539 passed, 0 failed, 2 errored, 11 broken→ exit 1./bin/install -yon Julia 1.13.0: exit 0 in 36 s. The live manifest is byte-identical to the.default, and~/.bashrcand the juliaup default were unchanged (compared before and after)JULIAUP_CHANNEL=1.12 ./bin/install -y: exit 0 (14 min, cold precompile cache), no precompile failuresJULIAUP_CHANNEL=1.10 ./bin/install -y: exit 1 with juliaup's "not installed" message.-hand a bad flag behave as expectedusing KiteModels→v"0.11.17"fsfe/reusecontainer): the only files flagged are untracked box files (.agent/,LocalPreferences.toml,.JETLSConfig.toml)bin/update_default_manifest(soinstall --updatetoo), with a stubjuliaon the PATH that logs its arguments and marks the live manifest during "update": exit 0, the calls werePkg.update()then the five precompiles, and the mark arrived inManifest-v1.13.toml.default(restored afterwards). A realPkg.updatewas not run, because it would move the pinsbin/julia_majorchange:./bin/install -yexit 0 in 11 s (warm), manifest byte-identical to.default,~/.bashrcand juliaup default unchanged;JULIAUP_CHANNEL=1.10still exits 1agent ci-local, 1.13): FAIL attest_find_steady_state(test/test-kps4.jl:581, "solver returned non-finite values" afteriterations=247). That is find_steady_state! does not converge for KPS4 on Julia 1.13, and CI hides it #314, already onmain, and this PR does not cause it: the branch leavessrc/,test/,Project.tomland the.defaultmanifest as they are onorigin/main, the tests call nobin/script, andinclude("test/test-kps4.jl")in the worktree session reproduces find_steady_state! does not converge for KPS4 on Julia 1.13, and CI hides it #314 with the same iteration count. GitHub CI stays green only becauseCI=trueturns this error into@test_broken, which find_steady_state! does not converge for KPS4 on Julia 1.13, and CI hides it #314 describes.defaultthat no longer instantiates now fails loudly instead of being quietly re-resolved. That is intended, but a user with an outdated registry may see it.Scope
+79 / −592 across 5 files:
bin/installrewritten,bin/update_default_manifestupdated to match, the newbin/julia_major, theCLAUDE.mdinstall line, andchangelog.d/323-….md.Not included:
bin/run_juliakeeps its own version parse.copy_bin()copies it into user projects without its neighbours, so it cannot callbin/julia_major.bin/create_sys_imagestill runsjuliaup add/juliaup defaultand repeats the install steps. That is cleanup: bin/create_sys_image still runs juliaup add/default and duplicates bin/install #325.fail-fast: false, which needs its owncleanup:PR.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #323 · task
KiteModels.jl-323