Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pnpm-workspace.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,18 @@ minimumReleaseAgeExclude:
overrides:
tar: 7.5.15

supportedArchitectures:

Copy link
Copy Markdown
Contributor

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 ...

Suggested change
supportedArchitectures:
# Install native deps for all platforms, not just the current machine's, so the
# cross-platform bundle builds (scripts/build-cli-bundles.ts) can embed them.
supportedArchitectures:

os:
- linux
- darwin
- win32
cpu:
- x64
- arm64
libc:
- glibc
- musl

# Unfortunately, several parts of this project expect everything to be hoisted up in the node_modules directory...
# And fixing it would take longer than just enabling this and shedding a tear
nodeLinker: hoisted
Expand Down
52 changes: 42 additions & 10 deletions scripts/build-cli-bundles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: stale reference — the second commit moved this config from package.json to pnpm-workspace.yaml.

Suggested change
// `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build
// `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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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).

https://bun.com/docs/bundler/executables

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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: the replaceAll is silent if there is no KEYRING_PLACEHOLDER to replace in the fat js. That should not happen and would probably indicate a problem in the credentials.ts (typo in the KEYRING_PLACEHOLDER, ...) or during the build (possibly minimisation, content corruption, ...).

Let's check that the fat js includes at least one reference to the KEYRING_PLACEHOLDER and throw at build time.

if (!fatEntrypointContent.contains(KEYRING_PLACEHOLDER)) {
    throw ...
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: if, for any reason, the if (subpackage !== writtenSubpackage) { never passes, the proxy-agent patch at line 126 is now dropped (never written) as well.

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],
Expand Down
4 changes: 0 additions & 4 deletions src/commands/auth/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ const tryToLogin = async (token: string) => {
tokenLocation = 'your OS keyring';
} else if (process.env.APIFY_DISABLE_KEYRING === '1') {
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring disabled via APIFY_DISABLE_KEYRING)`;
} else if (process.env.APIFY_CLI_BUNDLE) {
// Bundle distributions ship without the OS keyring native module — see
// https://github.com/apify/apify-cli/issues for the tracking issue.
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring not available in bundle installs; install via npm for keyring storage, or set APIFY_DISABLE_KEYRING=1 to silence)`;
} else {
tokenLocation = `${AUTH_FILE_PATH()} (OS keyring unavailable; set APIFY_DISABLE_KEYRING=1 to silence)`;
}
Expand Down
28 changes: 25 additions & 3 deletions src/lib/credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: in detectInstallMethod, we first check if (process.env.APIFY_CLI_MARKED_INSTALL_METHOD) {.

Once we publish bundled versions to NPM, the APIFY_CLI_MARKED_INSTALL_METHOD will be npm, but the underlying source will be bundle, right?

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;
}

/**
Expand Down
12 changes: 12 additions & 0 deletions src/lib/typings/keyring-native-subpackage.d.ts
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: @napi-rs/keyring exports Entry. Re-export should be possible and avoid type drift (if the upstream Entry in @napi-rs/keyring ever change).

Suggested change
export class Entry {
constructor(service: string, account: string);
getPassword(): string | null;
setPassword(password: string): void;
deletePassword(): boolean;
}
export { Entry } from '@napi-rs/keyring';

}
Loading