install.ps1: honour RANDNLA_PROJECT_DIR, and add -Prefix and -ModifyEnvironment - #163
Merged
Merged
Conversation
…nvironment Brings the Windows installer's location handling in line with install.sh, which is what makes the shared RandNLA-project convention actually work on both platforms. RANDNLA_PROJECT_DIR was read nowhere in this script. Its precedence is now identical to install.sh's -- the flag, then the environment variable, then a sibling of the clone -- so a machine that has already installed one RandNLA project does not scatter a second tree somewhere else. -ModifyEnvironment persists that variable for the user with [Environment]::SetEnvironmentVariable(..., "User"), the Windows equivalent of install.sh's --modify-rc and the only mechanism that survives opening a new shell; setting $env: alone would last for the current process. Opt-in, matching install.sh: the default touches nothing and prints the setx command instead. -Prefix installs RandLAPACK itself somewhere other than <ProjectDir>\install\RandLAPACK-install, for a module tree or any prefix a site wants to own. Dependencies stay in the project directory. Also moved the project-directory length warning to after path resolution. It was guarded on `$ProjectDir -ne ""`, so it only ever fired for an explicitly passed -ProjectDir -- never for the default, which is the common case and is derived from wherever the clone happens to sit, so the more likely one to be long. Verified on Windows 11 under Windows PowerShell 5.1 with VS 2022 Build Tools: all three precedence cases resolve as intended (sibling default, environment variable honoured, flag beating the variable), and the User-scope environment write round-trips. The oneMKL discovery, BLAS link check and space-free import library staging from #156 were all observed still working along the way.
Contributor
Author
|
For the record: this shows as merged rather than closed because #162's branch was fast-forwarded to contain it, so its commits are genuinely in that branch — it merged into its stacked base, not into Max asked for #162, #163 and #164 to land as one PR, matching how the RandBLAS installer set was consolidated. All of this work is now reviewable in #162. |
mmelnich
added a commit
that referenced
this pull request
Aug 13, 2026
… backend selection, and docs (#162) ## Problem RandLAPACK's two installers had drifted apart. `install/install.ps1` was rebuilt in #156 and pins every dependency, checksums downloads, records provenance and link-and-run validates the BLAS before building anything. `install/install.sh` did none of that, and the documentation described a script that no longer existed in several respects. Eleven defects, all verified against `main` before touching anything: | Where | Defect | |---|---| | `install.sh:336,339,342` | three `git clone` with **no ref** — blaspp, lapackpp, random123 all tracked upstream default branches | | — | **no BLAS validation at all** | | — | `/opt/homebrew` hardcoded, **8 places** — hard-fails on Intel macOS and custom `HOMEBREW_PREFIX` | | `install.sh:331` | `mv "$REPO_DIR"` relocated the user's own clone, breaking git worktrees | | `install.sh:211` | `: > "$LOG"` truncated the log, destroying the previous run's output | | `install.sh:247` | libdir search omitted `lib/aarch64-linux-gnu` | | — | no `--prefix`; extras and benchmarks unconditional with no way to skip | | `install.ps1` | **zero** references to `RANDNLA_PROJECT_DIR`, no `--modify-rc` equivalent | | `INSTALL_SCRIPT.md` | documented the pre-backend flag list, and still told people the script would move their clone | ## The two things most worth reviewing ### Integer width is read back, not assumed BLAS++ probes `int32` before `int64`, and `blas_int` only filters which *library names* to try. For MKL that is a real choice — `mkl_intel_lp64` and `mkl_intel_ilp64` are different libraries. For OpenBLAS there is only `-lopenblas`, so **a successful `blas_int=int64` configure proves nothing**: an LP64 build passes the `int32` probe and is accepted. The resolved width therefore comes from BLAS++'s generated `blas/defines.h` after the build. Trusting the request would stamp LP64 installs as ILP64 and have every later run reuse them believing otherwise. ### Verification runs the program, and that is the point The conftest compiles, links and *runs* against the finished install — through BLAS++ and LAPACK++ rather than raw `dgemm_`, since that is how RandLAPACK reaches them — checking a `gemm` result and a `gesdd` factorization numerically. `gesdd` is chosen because it is the routine Apple's legacy Accelerate computes incorrectly. Running it rather than only linking matters for a reason specific to integer width. BLAS++ and LAPACK++ *do* guard the `int64_t` downcast — `to_blas_int` (`blaspp/src/blas_internal.hh:16-22`) throws rather than truncates — but that guard keys off `sizeof(blas_int)` **as declared by the header**. If the headers say 64-bit while the library actually loaded is LP64, the guard compiles out and 64-bit values reach routines reading 32 bits. That surfaces not as wrong numbers but as nonsense control values: a misread workspace query becomes an absurd `lwork`, and the run dies in allocation or never finishes. Nothing catches it by inspection. ## A cross-project collision, found while testing Dependency install directories now carry backend and GPU in their names (`blaspp-mkl-cpu-install`). That is not cosmetic. RandBLAS's installer uses the same `RandNLA-project` layout and named its BLAS++ install **identically** (`blaspp-<backend>-install`), with a different stamp filename. With a shared `RANDNLA_PROJECT_DIR`, this script would rebuild over RandBLAS's BLAS++ — their stamp never matches ours — and **RandBLAS's next run would reuse an artifact we had replaced underneath it, while its own stamp still described the original.** Distinct names make it impossible; sharing is instead explicit via `BLASPP_INSTALL_DIR`, verified by the conftest. ## Everything else - **macOS keeps Homebrew OpenBLAS as the default.** Apple's legacy Accelerate has a broken divide-and-conquer `gesdd`, and RandLAPACK calls `gesdd` in `rl_rsvd.hh:146`, `rl_abrik.hh:692`, `rl_revd2.hh:208`, `rl_preconditioners.hh:355` and `rl_util.hh:413`. That is what #157's quarantine is about. `--blas=accelerate` warns, citing #159. The hardcoded `/opt/homebrew` is fixed regardless. - **Extras and benchmarks stay built by default** (`--no-extras` / `--no-benchmarks`), unlike RandBLAS's examples — they need nothing this script has not already built. - **The clone is never moved**; a symlink gives the same layout and the path CI invokes still resolves. - **RandBLAS is untouched** — still a pinned submodule, still authoritative. - **`install.ps1`** now honours `RANDNLA_PROJECT_DIR` with the same precedence as `install.sh`, gains `-ModifyEnvironment` (User-scope, the only thing that survives a new shell) and `-Prefix`, and its path-length warning now checks the resolved path rather than only an explicitly passed one. - **Progress rendering** in three tiers, determinate from the build tool's own output, with tier 0 byte-identical to the plain step list — plus a CI assertion that redirected output carries no escape sequence. - **`INSTALL_SCRIPT.md`** rewritten: current flags, corrected layout diagram, and a new section 6 with a tested-configuration table drawn from the CI lanes, the per-backend integer width, whether LP64 actually limits RandLAPACK, and why macOS is on OpenBLAS. ## Verification Local Linux (gcc 15.2, oneAPI MKL, NVIDIA GPU present): | Scenario | Result | |---|---| | Fresh MKL build, `--no-gpu` | 9/9 steps, ILP64 confirmed from `blas/defines.h` | | Re-run in place | all six dependency steps reused, 1s rebuild | | Full default run (extras + benchmarks) | 13/13 steps | | `--no-gpu` → `--gpu` | BLAS++ provenance invalidated, rebuilt rather than reused | | Dependency discovery | all three reused, 3/3 steps | | Run from a git worktree | clone left in place, worktree still functional | | Output redirected | no ANSI escapes, no carriage returns | | **Full test suite** | **313/313 pass** | | RandBLAS then RandLAPACK, shared `RANDNLA_PROJECT_DIR` | both dependency sets intact | | RandLAPACK reusing RandBLAS's BLAS++ | reused, only LAPACK++ built, 5/5 steps | Windows 11, Windows PowerShell 5.1, VS 2022 Build Tools: all three `-ProjectDir` precedence cases resolve as intended, and the User-scope environment write round-trips. ## RandBLAS submodule bump Also bumps the vendored RandBLAS from `04f2018` to `952251c` (RandBLAS main), and `RandLAPACK_RandBLAS_PIN` with it. The three commits in that range are **#185** (vcpkg manifest fix), **#186** (CMake hygiene) and **#187** (installer scripts). None of them touch `RandBLAS/` headers — the library code is byte-identical, and everything in the range is build machinery. Two pieces of it matter here: - RandBLAS's CMake floor moved 3.12 → 3.21. RandLAPACK already declares 3.21, so nothing to change; the submodule now simply enforces what RandLAPACK already required. - RandBLAS exports `randblas_stage_runtime_dlls()` from its installed package and prints a configuration summary. As a subproject under RandLAPACK, which configures it with `BUILD_TESTS=OFF`, that summary reports tests as deliberately skipped rather than warning. `CMakeLists.txt` enforces that the pin variable and the submodule move together, and it means it: the cross-check compares the variable against `git ls-tree HEAD RandBLAS`, so a staged-but-uncommitted bump fails configure until both land in one commit. Worth knowing if you ever bump this iteratively — it is not satisfiable before committing. Verified: clean build against the bumped submodule, and the full suite re-run. ## Notes for reviewers - **Consolidated from #162 + #163 + #164** at Max's request, matching how the RandBLAS installer set was combined. Those two show as *merged* rather than closed because this branch was fast-forwarded to contain them — they merged into their stacked base, not into `main`. The tree is byte-identical to the state tested and green as three separate PRs. - Stacked on #161 (CI fixes), which stays separate — it is independent and useful on its own. - **`-ModifyEnvironment` is not verified end to end.** It runs at the end of a successful install, and I could not reach it: the clone sits on a `\\wsl.localhost` path and Windows `git` refuses it with `dubious ownership`. Precedence and the User-scope write are each verified directly, but not together in one completed run. - Two upstream gaps are filed rather than worked around silently: BLAS++ never looks for an ILP64 OpenBLAS (#166), and implements only Apple's legacy Accelerate interface (#165).
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.
Problem
install.ps1readRANDNLA_PROJECT_DIRnowhere. That variable is the shared convention between this installer,install.shand RandBLAS's installer — it is what lets one machine keep a singleRandNLA-projecttree instead of scattering several. On Windows it simply did not exist, so-ProjectDirwas the only way to say where anything went, and there was no equivalent ofinstall.sh's--modify-rcto make a choice persist.There was also no
-Prefix, so RandLAPACK could only ever install to<ProjectDir>\install\RandLAPACK-install.What this PR does
RANDNLA_PROJECT_DIRis honoured, with precedence identical toinstall.sh: the flag, then the environment variable, then a sibling of the clone.-ModifyEnvironmentpersists it via[Environment]::SetEnvironmentVariable(..., "User")— the Windows equivalent of appending to a shell profile, and the only mechanism that survives opening a new shell (setting$env:alone lasts for the current process). Opt-in, matchinginstall.sh: the default touches nothing and prints thesetxline.-Prefixinstalls RandLAPACK itself elsewhere — a module tree, or any prefix a site wants to own. Dependencies stay in the project directory.$ProjectDir -ne "", so it only fired for an explicitly passed-ProjectDir— never for the default, which is derived from wherever the clone happens to sit and is therefore the more likely one to be long.Verification
On Windows 11, Windows PowerShell 5.1, VS 2022 Build Tools:
...\worktrees\RandNLA-project(sibling default)$env:RANDNLA_PROJECT_DIR = C:\rl-from-envC:\rl-from-env-ProjectDir C:\rl-from-flagwith the env var still setC:\rl-from-flagSetEnvironmentVariable(..., "User")round-tripWorth noting incidentally: those runs exercised the #156 machinery on the way through, and oneMKL discovery, the BLAS/LAPACK link check, and the space-free import-library staging (for
C:\Program Files (x86)\Intel\oneAPI\...) were all observed still working.Notes for reviewers
-ModifyEnvironmentruns at the end of a successful install, and I could not reach it from this setup: the clone lives on a\\wsl.localhostpath, and Windowsgitrefuses it withfatal: detected dubious ownership, which stops the dependency step. So the precedence logic and the User-scope write are each verified directly, but not the two together in one completed run. Worth someone doing on a Windows-native clone before merge, or I can if you'd rather.