-
Notifications
You must be signed in to change notification settings - Fork 47
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
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`. | ||||||
| // `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build | ||||||
|
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. Nit: stale reference — the second commit moved this config from
Suggested change
|
||||||
| // 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,21 +114,23 @@ 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(); | ||||||
| // 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)`, | ||||||
| ); | ||||||
|
|
||||||
| const newEntrypointResultFileContent = entrypointResultFileContent.replace( | ||||||
| `(0, utils_1.dynamicNodeImport)("proxy-agent")`, | ||||||
| `Promise.resolve().then(() => import_proxy_agent)`, | ||||||
| ); | ||||||
|
|
||||||
| await writeFile(entrypointResultFilePath, newEntrypointResultFileContent); | ||||||
| } | ||||||
| // The on-disk fat JS only varies by keyring subpackage, so we rewrite it once per subpackage | ||||||
| // rather than once per target (baseline variants share their sibling's subpackage). | ||||||
| let writtenSubpackage: string | undefined; | ||||||
|
|
||||||
| for (const target of targets) { | ||||||
| // eslint-disable-next-line prefer-const -- somehow it cannot tell that os and arch cannot be "const" while the rest are let | ||||||
|
|
@@ -141,6 +165,14 @@ 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`. Skip the rewrite when the on-disk file already targets this subpackage. | ||||||
| const subpackage = keyringSubpackage(os, arch, Boolean(musl)); | ||||||
| if (subpackage !== writtenSubpackage) { | ||||||
| await writeFile(entrypointResultFilePath, fatEntrypointContent.replaceAll(KEYRING_PLACEHOLDER, subpackage)); | ||||||
|
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. Suggestion: the Let's check that the fat js includes at least one reference to the
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. Important: if, for any reason, the Should not happen, AFAIK, but good to keep in mind and maybe future-proof. |
||||||
| writtenSubpackage = 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 🤔 |
||
| 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,12 @@ | ||||||||||||||||
| // 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. | ||||||||||||||||
| declare module '__APIFY_KEYRING_NATIVE_SUBPACKAGE__' { | ||||||||||||||||
| export class Entry { | ||||||||||||||||
| constructor(service: string, account: string); | ||||||||||||||||
| getPassword(): string | null; | ||||||||||||||||
| setPassword(password: string): void; | ||||||||||||||||
| deletePassword(): boolean; | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+6
to
+11
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. Nit:
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: let's comment on why we do this...
Something like ...