-
Notifications
You must be signed in to change notification settings - Fork 49
feat(auth): use OS keyring in bundle distributions #1197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
80e925f
c97c0d1
658d6c9
4795779
db4902f
4ee0153
1ffccb1
40287da
0b2c7b0
deea1e4
57b43e2
4c84daa
ef6b422
82fffa7
4c0c8ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,6 +63,28 @@ const entryPoints = [ | |
| fileURLToPath(new URL('../src/entrypoints/actor.ts', import.meta.url)), | ||
| ]; | ||
|
|
||
| // Placeholder specifier that `credentials.ts` imports for the OS keyring in bundle mode. | ||
| // Kept external in the fat-JS step so the literal `import()` survives, then rewritten per | ||
| // target below to the matching `@napi-rs/keyring-<platform>` subpackage so Bun's `--compile` | ||
| // embeds that one native `.node`. Must match the specifier in `src/lib/credentials.ts`. | ||
| const KEYRING_PLACEHOLDER = '__APIFY_KEYRING_NATIVE_SUBPACKAGE__'; | ||
|
|
||
| // Maps the compiled (os, arch, libc) to the napi-rs keyring subpackage that ships its `.node`. | ||
| // `supportedArchitectures` (pnpm-workspace.yaml) forces all of these into node_modules at build | ||
| // time so each target can resolve its own, regardless of the build machine's platform. | ||
| function keyringSubpackage(os: string, arch: string, musl: boolean): string { | ||
| switch (os) { | ||
| case 'linux': | ||
| return `@napi-rs/keyring-linux-${arch}-${musl ? 'musl' : 'gnu'}`; | ||
| case 'darwin': | ||
| return `@napi-rs/keyring-darwin-${arch}`; | ||
| case 'windows': | ||
| return `@napi-rs/keyring-win32-${arch}-msvc`; | ||
| default: | ||
| throw new Error(`No @napi-rs/keyring subpackage known for ${os}-${arch}`); | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+75
to
+87
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the main hack, --compile (bun) skipped napi-rs due to its dynamic require shenanigans -> we provide literal for specific bundle which is shipped as external subpackage (per arch). |
||
| await rm(new URL('../bundles/', import.meta.url), { recursive: true, force: true }); | ||
|
|
||
| // #region Inject the fact the CLI is ran in a bundle, instead of installed through npm/volta | ||
|
|
@@ -92,20 +114,24 @@ for (const entryPoint of entryPoints) { | |
| conditions: 'node', | ||
| target: 'bun', | ||
| sourcemap: 'none', | ||
| // Keep the keyring placeholder literal `import()` intact so it can be rewritten and | ||
| // resolved per target in step 2 (Bun only embeds a `.node` when --compile resolves it). | ||
| external: [KEYRING_PLACEHOLDER], | ||
| }); | ||
|
|
||
| const entrypointResultFilePath = result.outputs[0]!.path; | ||
|
|
||
| // Fix apify client js (it now lazy loads proxy-agent, which makes bun skip it from the bundle) | ||
| { | ||
| const entrypointResultFileContent = await result.outputs[0]!.text(); | ||
|
|
||
| const newEntrypointResultFileContent = entrypointResultFileContent.replace( | ||
| `(0, utils_1.dynamicNodeImport)("proxy-agent")`, | ||
| `Promise.resolve().then(() => import_proxy_agent)`, | ||
| ); | ||
|
|
||
| await writeFile(entrypointResultFilePath, newEntrypointResultFileContent); | ||
| // Fix apify client js (it now lazy loads proxy-agent, which makes bun skip it from the bundle). | ||
| // Kept in memory only — the per-target write below is what lands on disk before each compile. | ||
| const fatEntrypointContent = (await result.outputs[0]!.text()).replace( | ||
| `(0, utils_1.dynamicNodeImport)("proxy-agent")`, | ||
| `Promise.resolve().then(() => import_proxy_agent)`, | ||
| ); | ||
|
|
||
| // `replaceAll` is silent if the placeholder is gone, shipping a bundle that falls back to | ||
| // plaintext storage. Validate once up front and fail loud. | ||
| if (!fatEntrypointContent.includes(KEYRING_PLACEHOLDER)) { | ||
| throw new Error(`Keyring placeholder "${KEYRING_PLACEHOLDER}" not found in the fat JS for ${cliName}.`); | ||
| } | ||
|
|
||
| for (const target of targets) { | ||
|
|
@@ -141,6 +167,12 @@ for (const entryPoint of entryPoints) { | |
|
|
||
| console.log(`Building ${cliName} for ${target} (result: ${fileName})...`); | ||
|
|
||
| // Point the keyring import at this target's native subpackage so --compile embeds its | ||
| // `.node`. Rewrite every iteration so the compiled file is never the stale, unpatched | ||
| // Bun output — the proxy-agent fix above lands on disk here too, not just the keyring swap. | ||
| const subpackage = keyringSubpackage(os, arch, Boolean(musl)); | ||
| await writeFile(entrypointResultFilePath, fatEntrypointContent.replaceAll(KEYRING_PLACEHOLDER, subpackage)); | ||
|
|
||
| // Step 2: create the final executable bundle | ||
| await build({ | ||
| entrypoints: [entrypointResultFilePath], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import process from 'node:process'; | |
|
|
||
| import { AUTH_FILE_PATH } from './consts.js'; | ||
| import { ensureApifyDirectory } from './files.js'; | ||
| import { useCLIMetadata } from './hooks/useCLIMetadata.js'; | ||
| import { cliDebugPrint } from './utils/cliDebugPrint.js'; | ||
|
|
||
| const KEYRING_SERVICE = 'com.apify.cli'; | ||
|
|
@@ -41,15 +42,36 @@ export function __resetCredentialsForTests() { | |
|
|
||
| async function loadKeyringModule(): Promise<KeyringModule | null> { | ||
| if (cachedKeyringModule !== undefined) return cachedKeyringModule; | ||
| cachedKeyringModule = await importKeyringModule(); | ||
| return cachedKeyringModule; | ||
| } | ||
|
|
||
| async function importKeyringModule(): Promise<KeyringModule | null> { | ||
| // Bundle distributions can't load the `@napi-rs/keyring` wrapper: its createRequire-based | ||
| // platform loader isn't followed by Bun's `--compile`, so the native module never makes it | ||
| // into the binary. Instead each bundle embeds exactly one platform subpackage, and the | ||
| // specifier below is rewritten to it at build time (see scripts/build-cli-bundles.ts). | ||
| if (useCLIMetadata().installMethod === 'bundle') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: in Once we publish bundled versions to NPM, the That will break this check AFAIK 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bundles are not shiped to npm, install.sh download them from release artefacts -> https://github.com/apify/apify-cli/releases/tag/v1.6.2
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, now they are not shipped through npm, but, and correct me if I'm wrong, there either was or still is a plan to ship bundles through npm instead of plain JS. At least the comment above following check in In such case, the That being said, I think this is not an issue as the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I see your point now, lets remove that. |
||
| try { | ||
| const mod = (await import('__APIFY_KEYRING_NATIVE_SUBPACKAGE__')) as Partial<KeyringModule> & { | ||
| default?: Partial<KeyringModule>; | ||
| }; | ||
| const Entry = mod.Entry ?? mod.default?.Entry; | ||
| return Entry ? { Entry } : null; | ||
| } catch (err) { | ||
| cliDebugPrint('credentials', 'failed to load bundled keyring', err); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| // Indirect specifier so tsc doesn't try to resolve the module at compile time. | ||
| const specifier = '@napi-rs/keyring'; | ||
| cachedKeyringModule = (await import(specifier)) as KeyringModule; | ||
| return (await import(specifier)) as KeyringModule; | ||
| } catch (err) { | ||
| cliDebugPrint('credentials', 'failed to load @napi-rs/keyring', err); | ||
| cachedKeyringModule = null; | ||
| return null; | ||
| } | ||
| return cachedKeyringModule; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // The specifier below is a build-time placeholder. `scripts/build-cli-bundles.ts` | ||
| // rewrites it to the platform-specific `@napi-rs/keyring-<target>` subpackage for each | ||
| // compiled bundle, so Bun's `--compile` embeds that one native `.node`. Outside bundles | ||
| // the import is never executed. This declaration just keeps tsc/oxlint happy. | ||
| // | ||
| // The type is sourced from the `@napi-rs/keyring` wrapper, which is decoupled from runtime | ||
| // resolution (the bundle rewrites the specifier in the fat JS; the `.d.ts` is never read by | ||
| // the bundler). The wrapper re-exports the same NAPI `Entry` the native subpackage binds, so | ||
| // this stays accurate for free. Re-exporting from the subpackage itself wouldn't work — those | ||
| // ship only the `.node` binary with no `.d.ts`, so `Entry` would silently degrade to `any`. | ||
| declare module '__APIFY_KEYRING_NATIVE_SUBPACKAGE__' { | ||
| export type { Entry } from '@napi-rs/keyring'; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.