Repository navigation
Conversation
-y uses a supported Julia already installed via JULIAUP_CHANNEL instead of refusing the active one; supported versions are those with a tracked Manifest-v<major>.toml.default. Tests only with --tests. Nothing outside the repo changes: no juliaup default, no Revise in @v#.#, no shell alias. --update runs Pkg.update on the live manifest. bin/run_julia picks the channel the same way and loads Revise only where installed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 1.11 default manifest is the examples manifest, which the root project has to resolve down to its own dependencies before it can instantiate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: REQUEST CHANGES · 2 inline, 0 off the diff
Good
- The plan and the diff agree.
-ynow goes throughselect_julia_channel,juliaup defaultis gone, Revise no longer goes into@v#.#, the.bashrc/.zshrcalias is gone, and tests run only with--tests. I checked each against the card's 'What changed' list. - Failures now stop the script as the card says:
bin/installstarts with#!/bin/bash -eu, so a failedPkg.instantiateexits beforeInstallation complete!and beforeinstall_version_*.txtis written. - The supported versions come from one place:
SUPPORTED_JULIAis built from the trackedManifest-v*.toml.defaultfiles (on this branch: 1.11 and 1.12). That removes the hard-coded 1.11/1.12 checks and the menu. - In
bin/run_julia,select_julia_channelruns beforejulia_majoris computed at its line 39. The system image andinstall_versionlookup therefore follow the chosen channel, not the active 1.13. --updateno longer deletes the manifest. It seeds from the.defaultonly when no live manifest exists, as AGENTS.md §2 asks.- The
Reviseguard (Base.find_package("Revise") === nothing || @eval using Revise) means removing the global Revise install cannot break the REPL. - The changelog fragment marks the
--no-testsremoval BREAKING and lists every behaviour change visible to users.
Not good
bin/install:86— On 1.12install_projects=()is empty. Under-u,"${install_projects[@]}"aborts with 'unbound variable' on bash older than 4.4, which is the macOS system/bin/bash(3.2). Install therefore fails on every 1.12 run on macOS, a platform the old script handled explicitly.bin/run_julia:24— This file now hasactive_juliaavailable but still computesjulia_majorby slicingjulia --versionat line 39. That is two codepaths for the same quantity in a file this PR opens. §2 says to unify them here.- The rewrite drops several behaviours the card does not name: the macOS
MPLBACKEND=qtaggexport before precompile and the smoke load, the Linux MakieControlPlots load warning, and the 'juliaup is not installed' help with per-OS install commands. Each is either deliberate or a regression, and the reviewer cannot tell which. - The disk check no longer guards an empty
dfresult. Under-uan emptyavailable_kbevaluates to 0, so the user gets a false 'not enough free disk space' error instead of 'could not determine'. - On 1.12 the script no longer removes
examples/Manifest-v1.12.toml. Any leftover from an earlier install now sits next to the workspace manifest without being cleaned up. - The two-line comment at
bin/install:77-78explains why the 1.11 branch is shaped as it is. Under §3 that reasoning belongs in the PR card, where it already is. active_juliastarts a whole Julia process just to print major.minor.bin/run_julianow pays for that on every launch, on top of its ownjulia --versioncall.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| for _label in "${RETRIED_RESOLVES[@]}"; do | ||
| echo " - ${_label}" | ||
| done | ||
| for project in . "${install_projects[@]}"; do |
There was a problem hiding this comment.
MAJOR: On 1.12 install_projects=() is empty. Under -u, "${install_projects[@]}" aborts with 'unbound variable' on bash older than 4.4, which is the macOS system /bin/bash (3.2). Install therefore fails on every 1.12 run on macOS, a platform the old script handled explicitly.
There was a problem hiding this comment.
Fixed in 347c87d: the list is now (. examples) on 1.11 and (.) otherwise, so no empty array is expanded. Not run on bash 3.2.
|
Local full suite: PASS (1 min, Julia 1.13.0, one cell of the matrix) |
…t guards - Never expand an empty project array (aborts under -u on macOS bash 3.2). - active_julia reads julia --version; run_julia uses it instead of slicing. - Guard an empty df result; remove leftover member manifests on 1.12. - MPLBACKEND on macOS lives in setup_env, shared by install and run_julia. - Point at the juliaup installer when no supported Julia is found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
347c87d, one line per finding. Dropped behaviours: MPLBACKEND restored, moved into setup_env so install and run_julia share one copy. The MakieControlPlots warning is dropped on purpose because the smoke load now fails the install with the real error. The per-OS juliaup commands are replaced by one julialang.org/install line, printed only when juliaup is missing. Empty df: restored the 'could not determine' error. Leftover examples/test Manifest-v1.12.toml: removed on 1.12 again. 77-78 comment: cut to one line saying 1.11 has no workspaces; the reasoning stays on the card. active_julia: now parses julia --version, 20 ms against 330 ms. Re-ran ./bin/install -y afterwards: 1.12.7, exit 0. |
TL;DR
./bin/install -ynow falls back to the newest installed Julia that has a trackedManifest-v<major>.toml.default, throughJULIAUP_CHANNEL, instead of refusing the active 1.13. It no longer runs tests by default, writes to@v#.#or~/.bashrc, or deletes the manifest on--update. Without that, no worktree of this repo could be installed on the box.What was wrong
On the box (
juliaup status: 1.11.9, 1.12.7, default *1.13.0),-ytook the active 1.13 and stopped atError: Julia 1.13 is not supported. Only Julia 1.11 and 1.12 are supported.The script also drifted from AGENTS.md §2 in the other three ways the issue lists:Pkg.test()by default,Pkg.add("Revise")into the shared environment plus ajlalias appended to~/.bashrc/~/.zshrc, and--updatedeleting the manifest beforePkg.update.What changed
bin/setup_env):active_juliareadsjulia --version(20 ms, against 330 ms for starting Julia to printVERSION), andrun_julianow uses it instead of its own string slicing.select_julia_channelkeeps the active Julia if it is supported, and otherwise exportsJULIAUP_CHANNELas the first installed supported channel. A version counts as supported when the repo tracksManifest-v<major>.toml.defaultfor it, so the version list lives in one place. Adding a 1.13 manifest (Support Julia 1.13 in KiteControllers #73) makes 1.13 supported with no script edits. Without-y, the menu still offers the versions and runsjuliaup add, but neverjuliaup default.bin/install: copies the.defaultover the live manifest, instantiates, precompilesexamplesand smoke-loads it.--testsis the only way to get the test suite;--no-testsand the question about tests are gone, which the changelog marks BREAKING.--updaterunsPkg.updateon the live manifest and seeds it from the.defaultonly when it is missing. I dropped the clean-retry that deleted the manifest and resolved from scratch after a failure: it silently replaced the pins, and now a failure stops the script. The script went from ~590 lines to ~120, mostly because of the duplicated registry and retry blocks.dfgives nothing),MPLBACKEND=qtaggon macOS (moved intobin/setup_env, so install andrun_juliashare one copy), and removing leftover member manifests on 1.12. Dropped on purpose: the Linux "MakieControlPlots could not be loaded" warning, because the smoke load now stops the install with the real error instead; and the per-OS juliaup install commands, replaced by one line pointing at https://julialang.org/install that is printed only when no supported Julia is found and juliaup is missing. juliaup is no longer required when the active Julia is already supported..defaultis the examples manifest (that is whatbin/update_default_manifestcopies). So the root andexampleseach get a copy (the project list is(. examples)against(.)on 1.12, which never expands an empty array, since macOS's bash 3.2 aborts on one under-u) and runPkg.resolve(); Pkg.instantiate(), as before. On 1.12 the workspace manifest coversexamples/test/docs, and instantiate alone is enough.bin/run_juliachooses the Julia the same way (preferring versions that have aninstall_version_*.txt), and runsusing Reviseonly when Revise is findable. Otherwise, removing Revise from the global install would break the REPL for a user who never installed it.Where I'd push back
juliaserversession launches a plainjulia, so it runs 1.13.0 for this repo however the install picks. I checked:Pkg.activate(examples); using KiteControllersin the session fails withPackage KiteControllers ... does not seem to be installed. The repo side of that fix is Support Julia 1.13 in KiteControllers #73 (support 1.13), in line with Bart's comment on 1-Bart-1/Agents#629..defaultis stale againstexamples/Project.toml. The 1.11 resolve drops CondaPkg's chain (CondaPkg, MicroMamba, micromamba_jll, Pidfile, pixi_jll; CondaPkg was removed in d7ccbfd) and moves the path-sourced KiteControllers 0.2.28→0.2.31. Every registered pin holds. I didn't regenerate the manifest here, because that is a PR of its own.bin/update_default_manifeststill runsjuliaup default, which is the same drift in the next script over. I left it out to keep this PR to one script's contract.Verification
./bin/install -yon main →Error: Julia 1.13 is not supported., exit 1 (box log in the thread)./bin/install -ywith default 1.13 →Using Julia 1.12.7,Installation complete!, exit 0; liveManifest-v1.12.tomlbyte-identical to the.defaultJULIAUP_CHANNEL=1.11 ./bin/install -y→Using Julia 1.11.9, exit 0 (red first:failed to find source of parent package: "IntervalArithmetic"before the 1.11 resolve was restored)./bin/install -y(default 1.13) →Using Julia 1.12.7,Installation complete!, exit 0; live manifest identical to the.default;~/.bashrcand@v1.1{1,2,3}hashes unchanged~/.bashrcand~/.julia/environments/v1.1{1,2,3}/Project.tomlidentical before and after all three runs-hprints usage;--no-testsand unknown flags exit 1 with usage;select_julia_channel 9.9→ 1;select_julia_channel 1.11→JULIAUP_CHANNEL=1.11--updatenot run: it wouldPkg.updatethis worktree's live manifest, which §2 rules out by hand--testsnot run (it isPkg.test())Pkg.test()on the box's Julia 1.13.0, which this repo does not support, so its result says nothing about this change. GitHub CI does not callbin/install.select_julia_channelreadsjuliaup statusby column. A juliaup that changes that table would make-yfail with the "no Julia with a default manifest" error, not pick the wrong version.Scope
+129 / −546 across 5 files:
bin/installrewritten to the contract, the channel helper inbin/setup_env,bin/run_juliausing it, the changelog fragment, and.gitignorewidened fromexamples/LocalPreferences.tomltoLocalPreferences.tomlin any project. The box writes the root one per worktree, and it is per-machine like the examples one.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #71 · task
KiteControllers.jl-71