Skip to content

build: run the build when installed from git - #47

Merged
shreyaskarnik merged 4 commits into
shreyaskarnik:mainfrom
Joilence:pr/prepare-script
Aug 30, 2026
Merged

build: run the build when installed from git#47
shreyaskarnik merged 4 commits into
shreyaskarnik:mainfrom
Joilence:pr/prepare-script

Conversation

@Joilence

@Joilence Joilence commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Why

main points at ./dist/index.js, but dist/ is gitignored. A git install therefore has no entry point:

$ npm install "git+https://github.com/shreyaskarnik/argo"
$ npx argo --version
ERR_MODULE_NOT_FOUND: .../@argo-video/cli/dist/index.js

Installs from npm are fine, since the tarball ships dist/ via files. That is the only path CI covers.

What

+  "prepare": "npm run build",

npm runs prepare for git dependencies after installing devDependencies, and typescript is already one. Tarball installs skip it, so npm users see no change.

Test

Same git URL both ways: without the fix dist/ is missing and the CLI crashes, with it argo --version prints 0.39.1. Suite: 753 pass.

Note: npm install here now builds too, so CI builds twice.

Joilence and others added 3 commits August 14, 2026 21:52
`dist/` is gitignored and `main` points at `./dist/index.js`, so
installing this package from a git URL produced a package whose entry
point did not exist. npm installs devDependencies and runs `prepare` for
git dependencies, and typescript is already a devDependency, so the
build just needs to be wired to that lifecycle hook.

Installing from the npm registry is unaffected: the published tarball
already ships `dist/`, and `prepare` does not run for registry installs
of a packed tarball.
`prepare` runs `build`, and `build` chained a POSIX-only `copy-assets`:

  mkdir -p dist/transitions/shaders && cp src/transitions/shaders/*.glsl ...

npm runs lifecycle scripts through cmd.exe on Windows, where `cp` does not
exist and `mkdir -p` creates a directory literally named `-p`. Before this
PR that only broke an explicit `npm run build`; with `prepare` it breaks
plain `npm install` too — including the very command this PR exists to fix,
`npm i git+https://github.com/shreyaskarnik/argo`, which would now fail
during prepare rather than at runtime. The README documents Windows
(choco install ffmpeg) and every CI job is ubuntu-latest, so nothing here
would have caught it.

A script file rather than an inline `node -e`, because quoting is the same
hazard one level down: cmd.exe and sh disagree about nested quotes, which is
what made the original break.

It copies only *.glsl. A recursive directory copy — the obvious one-liner —
also drags index.ts and a README into dist/, which the old glob did not.

Verified: build emits 5 shaders, `npm pack` carries all 5, `npm run prepare`
reproduces them from a clean dist/, 774 tests pass.
@shreyaskarnik

Copy link
Copy Markdown
Owner

Thanks @Joilence — good find, and the diagnosis in the description is exactly right. main points at ./dist/index.js, dist/ is gitignored, so a git install has no entry point at all. I confirmed both halves of that before looking at the fix.

Also verified the things a one-line prepare could plausibly have broken, and none of them did:

  • The files allowlist still wins, so the packed tarball carries all of dist/ and npm-tarball consumers see no change — npm only runs prepare for git deps.
  • npm pack --silent still writes just the tarball filename to stdout, so the pack-smoke job's TARBALL=$(pwd)/$(npm pack --silent) capture is unaffected. That was my main worry about a silent CI break.

I pushed one commit to your branch (d6119e0) rather than sending you round again — hope that's alright.

prepare chains copy-assets, which was POSIX-only:

mkdir -p dist/transitions/shaders && cp src/transitions/shaders/*.glsl ...

npm runs lifecycle scripts through cmd.exe on Windows, where cp doesn't exist and mkdir -p creates a directory literally named -p. Before this PR that only broke an explicit npm run build; with prepare it breaks plain npm install too — including, ironically, the exact command this PR exists to fix, which would fail during prepare instead of at runtime. The README documents Windows (choco install ffmpeg) and every CI job is ubuntu-latest, so nothing here would have caught it.

Two details in the replacement worth flagging, since both are easy to get wrong:

  • It's a script file, not an inline node -e. Quoting is the same hazard one level down — cmd.exe and sh disagree about nested quotes, which is what broke the original.
  • It filters to *.glsl. The obvious fs.cpSync(src, dst, { recursive: true }) one-liner also drags index.ts and a README into dist/, which the old glob didn't.

One thing worth a line in the README, not a change request. The presence of a prepare script is precisely what makes npm install the git dependency's full devDependencies before packing. Here that's ~719MB — onnxruntime-node alone is 208MB with a native-binary postinstall, and @huggingface/transformers another 143MB — none of which the build needs beyond typescript and the typings. There's no cheaper hook, since npm specifically looks for prepare on git deps, so it's inherent to the approach rather than something to fix. Just better documented than discovered.

Verified on the updated branch: build emits 5 shaders, npm pack carries all 5, npm run prepare reproduces them from a clean dist/, 774 tests pass.

`prepare` makes copy-assets run inside `npm pack`, and the pack-smoke job
captures that command's stdout as the tarball filename:

  echo "TARBALL=$(pwd)/$(npm pack --silent)" >> "$GITHUB_ENV"

The progress line landed on stdout, so the capture became two lines and the
step failed with:

  ##[error]Invalid format 'argo-video-cli-0.39.1.tgz'

stderr keeps the build feedback without contaminating a stream something
else parses. `--silent` suppresses npm's own output, not a lifecycle
script's, so any future addition to this path has the same constraint.

Worth noting the pack-smoke job caught this on its first real outing — the
failure only exists once `prepare` is wired, which is what that job is for.
@shreyaskarnik

Copy link
Copy Markdown
Owner

Correction to my last comment — that ended up being two commits, not one. The first push broke Packed tarball E2E, and the failure is worth recording because it's a genuinely non-obvious interaction:

echo "TARBALL=$(pwd)/$(npm pack --silent)" >> "$GITHUB_ENV"
##[error]Invalid format 'argo-video-cli-0.39.1.tgz'

My replacement script logged a progress line to stdout. Once prepare exists, copy-assets runs inside npm pack, and that job captures pack's stdout as the tarball filename — so the capture became two lines. --silent suppresses npm's own output, not a lifecycle script's.

Moved to stderr in the follow-up commit. npm pack --silent now yields exactly argo-video-cli-0.39.1.tgz again, and all six checks are green.

Two things I'd note from that, neither a change request:

  • I'd verified stdout was clean when reviewing your original — and it was, since cp prints nothing. Wiring prepare is what turned this path into something whose stdout is parsed, so the constraint arrived with the feature. Anything added to the build path from here has to respect it; the script carries a comment saying so.
  • The pack-smoke job caught this on its first real outing. That failure can only exist once prepare is wired, which is precisely the class of thing it was added for.

Sorry for the extra churn on your branch — the fix is entirely on my side of it, not yours.

@shreyaskarnik
shreyaskarnik merged commit f031abe into shreyaskarnik:main Aug 30, 2026
6 checks passed
Joilence added a commit to Joilence/argo that referenced this pull request Aug 30, 2026
Upstream merged shreyaskarnik#47 and shreyaskarnik#48, so dev's own drafts of the Gemini PCM work are
superseded. All five tts files resolve to upstream: diffing the two sides
showed every line unique to dev was something shreyaskarnik#48 had since fixed, namely the
`gemini-2.5-flash` default that answers an AUDIO request with 400, the
`describe.runIf` guard that skips silently instead of failing, and
`expect(header.audioFormat).toBe(3)`, which ffmpeg 6 fails because it tags
float32 as EXTENSIBLE.

package.json takes upstream's `node scripts/copy-assets.mjs` over the shell
one-liner, which upstream introduced alongside the `prepare` hook from shreyaskarnik#47:
`prepare` now runs on the installer's machine, where `mkdir -p` and `cp` are
not a given.
@Joilence
Joilence deleted the pr/prepare-script branch August 30, 2026 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants