Skip to content
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.

Sign up for GitHub

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

Jump to bottom

simplify package path resolution #8186

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

simplify package path resolution #8186

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
346d665
WIP
turadg Aug 14, 2023
e08e39a
more resolvePathname
turadg Aug 14, 2023
a54c366
move resolvePathname to swingset-vat
turadg Aug 14, 2023
49b01fb
fixup
turadg Aug 14, 2023
859ae99
build: drop extraneous import-meta-resolve
turadg Aug 14, 2023
2940517
less async
turadg Aug 14, 2023
f467a51
redundant awaits
turadg Aug 14, 2023
e0454b3
fixup
turadg Aug 15, 2023
e011f51
re-use import.meta.url for package resolution
turadg Aug 15, 2023
ba20dba
fixup
turadg Aug 15, 2023
67f2cf1
test refactor simplify
turadg Aug 15, 2023
a62b8a8
simplify
turadg Aug 15, 2023
9f9f7c1
fixup some migration to package import
turadg Aug 15, 2023
edb3e5f
thread import.meta.url
turadg Aug 25, 2023
2b709ef
try out replacing bfile
turadg Aug 25, 2023
979f444
Merge branch 'master' into ta/meta-resolve
turadg Aug 25, 2023
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
9 changes: 6 additions & 3 deletions packages/ERTP/test/swingsetTests/ertpService/test-ertp-service-upgrade.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';
// eslint-disable-next-line import/order
import { assert } from '@agoric/assert';
import { buildVatController } from '@agoric/swingset-vat';
import { makeResolvePath } from '@agoric/swingset-vat/tools/paths.js';

const bfile = name => new URL(name, import.meta.url).pathname;
const resolvePath = makeResolvePath(import.meta.url);

test('ertp service upgrade', async t => {
/** @type {SwingSetConfig} */
Expand All @@ -16,10 +17,12 @@ test('ertp service upgrade', async t => {
// defaultReapInterval: 1,
vats: {
// TODO refactor to use bootstrap-relay.js
bootstrap: { sourceSpec: bfile('bootstrap-ertp-service-upgrade.js') },
bootstrap: {
sourceSpec: resolvePath('./bootstrap-ertp-service-upgrade.js'),
},
},
bundles: {
ertpService: { sourceSpec: bfile('vat-ertp-service.js') },
ertpService: { sourceSpec: resolvePath('./vat-ertp-service.js') },
},
};

Expand Down
4 changes: 2 additions & 2 deletions packages/SwingSet/src/controller/initializeSwingset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from './bundle-handler.js';

import '../types-ambient.js';
import { pkgAbsPath } from '../../tools/paths.js';
import { resolvePath } from '../../tools/paths.js';

const trace = makeTracer('IniSwi', false);

Expand Down Expand Up @@ -174,7 +174,7 @@ export function loadBasedir(basedir, options = {}) {
*/
function resolveSpecFromConfig(referrer, specPath) {
try {
return pkgAbsPath(specPath);
return resolvePath(specPath, import.meta.url);
} catch (e) {
if (e.code !== 'MODULE_NOT_FOUND' && e.code !== 'ERR_MODULE_NOT_FOUND') {
throw e;
Expand Down
23 changes: 13 additions & 10 deletions packages/SwingSet/test/bundling/test-bundles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,9 @@ import { assert } from '@agoric/assert';
import { initSwingStore } from '@agoric/swing-store';
import { initializeSwingset, makeSwingsetController } from '../../src/index.js';
import { kunser, krefOf } from '../../src/lib/kmarshal.js';
import { makeResolvePath } from '../../tools/paths.js';

function bfile(name) {
return new URL(name, import.meta.url).pathname;
}
const resolvePath = makeResolvePath(import.meta.url);

// bundle IDs are hex SHA512, so this must be 128 characters long (plus the
// 'b1-' version prefix)
Expand All @@ -33,31 +32,35 @@ const bundleIDRE = new RegExp('^b1-[0-9a-f]{128}$');
test('bundles', async t => {
// we provide the bootstrap vat as a pre-made bundle, to exercise
// config.vats.NAME.bundle
const bootstrapBundle = await bundleSource(bfile('bootstrap-bundles.js'));
const bootstrapBundle = await bundleSource(
resolvePath('./bootstrap-bundles.js'),
);

// This bundle is not a vat, it exports 'runTheCheck()', and is imported by
// userspace. It exercises config.bundles.NAME.sourceSpec
const importableNonVatBundleFilename = bfile('nonvat-importable.js');
const importableNonVatBundleFilename = resolvePath('./nonvat-importable.js');

// This one (with 'hi()') provides a named vat bundle, for a static vat,
// exercising config.vats.NAME.bundleName . We also build dynamic vats to
// test D(devices.bundle).getNamedBundleCap and E(vatAdmin).createVatByName
const namedBundleFilename = bfile('vat-named.js');
const namedBundleFilename = resolvePath('./vat-named.js');

// We save this vat bundle (with 'disk()') to disk, to exercise
// config.bundles.NAME.bundleSpec
const diskBundle = await bundleSource(bfile('vat-disk.js'));
const diskBundle = await bundleSource(resolvePath('./vat-disk.js'));
const diskBundleID = `b1-${diskBundle.endoZipBase64Sha512}`;
const diskBundleFilename = bfile('bundle-disk.bundle');
const diskBundleFilename = resolvePath('./bundle-disk.bundle');
fs.writeFileSync(diskBundleFilename, JSON.stringify(diskBundle));
t.teardown(() => fs.unlinkSync(diskBundleFilename));

// We install this vat bundle at runtime, it provides 'runtime()'
const installableVatBundle = await bundleSource(bfile('vat-install.js'));
const installableVatBundle = await bundleSource(
resolvePath('./vat-install.js'),
);

// We install this non-vat bundle at runtime, it exports 'runTheCheck()'
const installableNonVatBundle = await bundleSource(
bfile('nonvat-install.js'),
resolvePath('./nonvat-install.js'),
);

const config = {
Expand Down
9 changes: 7 additions & 2 deletions packages/SwingSet/tools/paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,14 @@ import { resolve } from 'import-meta-resolve';

/**
* @param {string} specifier
Copy link
Member

Choose a reason for hiding this comment

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

This function should not be. Hiding the mechanism of resolution obscures the limitations of its use. I strongly prefer inlining url.filePathToURL(resolve(specifier, import.meta.url)) since this works anywhere without qualification.

If we kept this function, we would need to express the qualifications: it can only be reliably used for absolute module specifiers like @foo/bar and not for relative module specifiers like ./foo.js within this package. The resolution of an absolute specifier is not guaranteed to be consistent between packages, differing version constraints between packages, and varying node_modules layout algorithms.

The reason for these limitations is that it uses its own import.meta.url instead of receiving the referrer specifier as a parameter.

That is to say, if present, this function should not be exported to other packages, which is antithetical to exporting it from tools.js and its usage throughout this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

resolution of an absolute specifier is not guaranteed to be consistent between packages, differing version constraints between packages, and varying node_modules layout algorithms.

This I hadn't considered.

What if this function took parent the way import-meta-resolve does?

Copy link
Member

Choose a reason for hiding this comment

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

If this function accepted the import.meta.url from the caller, I would be converted from a “hard no” to a “soft no”, on the grounds that I’d prefer transparency at the call site. url.fileURLToPath(resolve(specifier, import.meta.url)) is very clear and not very long. Just removing the await is an improvement.

* @param {string} base
* @returns {string} file system absolute path of the package resource specifier
*/
export const pkgAbsPath = specifier => {
const href = resolve(specifier, import.meta.url);
export const resolvePath = (specifier, base) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe resolveToPath would be more clear. The arguments are both module specifiers and a path returns.

const href = resolve(specifier, base);
return url.fileURLToPath(href);
};

export const makeResolvePath = base => {
return specifier => resolvePath(specifier, base);
};
Copy link
Member

Choose a reason for hiding this comment

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

Alright, this is a good compromise.

@warner I would encourage us to in general migrate as much of our API surface to treating file URLs as first-class, starting with @endo/bundle-source and working our way out. @endo/compartment-mapper APIs are already framed in terms of file URLs.

4 changes: 2 additions & 2 deletions packages/boot/test/bootstrapTests/supports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { makeFakeStorageKit } from '@agoric/internal/src/storage-test-utils.js';
import { initSwingStore } from '@agoric/swing-store';
import { kunser } from '@agoric/swingset-liveslots/test/kmarshal.js';
import { loadSwingsetConfigFile } from '@agoric/swingset-vat';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { resolvePath } from '@agoric/swingset-vat/tools/paths.js';
import { E } from '@endo/eventual-send';
import { makeQueue } from '@endo/stream';
import { TimeMath } from '@agoric/time';
Expand Down Expand Up @@ -188,7 +188,7 @@ export const getNodeTestVaultsConfig = async (
bundleDir = 'bundles',
specifier = '@agoric/vm-config/decentral-itest-vaults-config.json',
) => {
const fullPath = pkgAbsPath(specifier);
const fullPath = resolvePath(specifier, import.meta.url);
const config = /** @type {SwingSetConfig & { coreProposals?: any[] }} */ (
await loadSwingsetConfigFile(fullPath)
);
Expand Down
4 changes: 2 additions & 2 deletions packages/boot/test/test-boot-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import { promises as fsPromises } from 'fs';
import path from 'path';

import { extractCoreProposalBundles } from '@agoric/deploy-script-support/src/extract-proposal.js';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { resolvePath } from '@agoric/swingset-vat/tools/paths.js';
import { mustMatch } from '@agoric/store';
import { loadSwingsetConfigFile, shape as ssShape } from '@agoric/swingset-vat';
import { provideBundleCache } from '@agoric/swingset-vat/tools/bundleTool.js';

const importConfig = configName =>
pkgAbsPath(`@agoric/vm-config/${configName}`);
resolvePath(`@agoric/vm-config/${configName}`, import.meta.url);

const test =
/** @type {import('ava').TestFn<Awaited<ReturnType<typeof makeTestContext>>>}} */ (
Expand Down
28 changes: 6 additions & 22 deletions packages/boot/test/upgrading/test-upgrade-contracts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/
import { test as anyTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';

import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { makeResolvePath } from '@agoric/swingset-vat/tools/paths.js';
import { buildVatController } from '@agoric/swingset-vat';

/**
Expand All @@ -14,39 +14,23 @@ import { buildVatController } from '@agoric/swingset-vat';
*/
const test = anyTest;

/**
* WARNING: uses ambient authority of import.meta.url
*
* We aim to use ambient authority only in test.before(); splitting out
* makeTestContext() lets us type t.context.
*/
const makeTestContext = async () => {
const bfile = name => new URL(name, import.meta.url).pathname;
const importSpec = spec => pkgAbsPath(spec);
return { bfile, importSpec };
};

test.before(async t => {
t.context = await makeTestContext();
});
const resolvePath = makeResolvePath(import.meta.url);

test('upgrade mintHolder', async t => {
const { bfile, importSpec } = t.context;

/** @type {SwingSetConfig} */
const config = harden({
bootstrap: 'bootstrap',
vats: {
// TODO refactor to use bootstrap-relay.js
bootstrap: { sourceSpec: bfile('./bootstrap.js') },
zoe: { sourceSpec: importSpec('@agoric/vats/src/vat-zoe.js') },
bootstrap: { sourceSpec: resolvePath('./bootstrap.js') },
zoe: { sourceSpec: resolvePath('@agoric/vats/src/vat-zoe.js') },
},
bundles: {
zcf: {
sourceSpec: importSpec('@agoric/zoe/contractFacet.js'),
sourceSpec: resolvePath('@agoric/zoe/contractFacet.js'),
},
mintHolder: {
sourceSpec: importSpec('@agoric/vats/src/mintHolder.js'),
sourceSpec: resolvePath('@agoric/vats/src/mintHolder.js'),
},
},
});
Expand Down
43 changes: 15 additions & 28 deletions packages/boot/test/upgrading/test-upgrade-vats.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,23 @@ import { test as anyTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava

import { BridgeId } from '@agoric/internal';
import { buildVatController } from '@agoric/swingset-vat';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { makeResolvePath } from '@agoric/swingset-vat/tools/paths.js';
import { makeRunUtils } from '../bootstrapTests/supports.js';

/**
* @type {import('ava').TestFn<
* Awaited<ReturnType<typeof makeTestContext>>
* >}
* @type {import('ava').TestFn<>}
*/
const test = anyTest;

const { Fail } = assert;

const importSpec = spec => pkgAbsPath(spec);

const makeTestContext = async metaUrl => {
const bfile = name => new URL(name, metaUrl).pathname;

return { bfile };
};
const resolvePath = makeResolvePath(import.meta.url);

const makeCallOutbound = t => (srcID, obj) => {
t.log(`callOutbound(${srcID}, ${obj})`);
return obj;
};

/** NOTE: limit ambient authority such as import.meta.url to test.before() */
test.before(async t => {
t.context = await makeTestContext(import.meta.url);
});

/**
* @param {any} t
* @param {Partial<SwingSetConfig>} [kernelConfigOverrides]
Expand All @@ -51,7 +38,9 @@ const makeScenario = async (
defaultReapInterval: 'never',
vats: {
bootstrap: {
sourceSpec: importSpec('@agoric/swingset-vat/tools/bootstrap-relay.js'),
sourceSpec: resolvePath(
'@agoric/swingset-vat/tools/bootstrap-relay.js',
),
},
},
bundleCachePath: 'bundles',
Expand All @@ -74,7 +63,7 @@ const makeScenario = async (
test('upgrade vat-board', async t => {
const bundles = {
board: {
sourceSpec: importSpec('@agoric/vats/src/vat-board.js'),
sourceSpec: resolvePath('@agoric/vats/src/vat-board.js'),
},
};

Expand Down Expand Up @@ -105,7 +94,7 @@ test('upgrade vat-board', async t => {
test.skip('upgrade bootstrap vat', async t => {
const bundles = {
chain: {
sourceSpec: importSpec('@agoric/vats/src/core/boot-chain.js'),
sourceSpec: resolvePath('@agoric/vats/src/core/boot-chain.js'),
},
};
// @ts-expect-error error in skipped test
Expand All @@ -130,12 +119,11 @@ test.skip('upgrade bootstrap vat', async t => {
});

test('upgrade vat-bridge', async t => {
const { bfile } = t.context;
const bundles = {
bridge: { sourceSpec: importSpec('@agoric/vats/src/vat-bridge.js') },
bridge: { sourceSpec: resolvePath('@agoric/vats/src/vat-bridge.js') },
};
const devices = {
bridge: { sourceSpec: bfile('./device-bridge.js') },
bridge: { sourceSpec: resolvePath('./device-bridge.js') },
};

const expectedStorageValues = ['abc', 'def'];
Expand Down Expand Up @@ -241,14 +229,13 @@ test('upgrade vat-bridge', async t => {
});

test('upgrade vat-bank', async t => {
const { bfile } = t.context;
const bundles = {
bank: { sourceSpec: importSpec('@agoric/vats/src/vat-bank.js') },
bridge: { sourceSpec: importSpec('@agoric/vats/src/vat-bridge.js') },
mint: { sourceSpec: bfile('./vat-mint.js') },
bank: { sourceSpec: resolvePath('@agoric/vats/src/vat-bank.js') },
bridge: { sourceSpec: resolvePath('@agoric/vats/src/vat-bridge.js') },
mint: { sourceSpec: resolvePath('./vat-mint.js') },
};
const devices = {
bridge: { sourceSpec: bfile('./device-bridge.js') },
bridge: { sourceSpec: resolvePath('./device-bridge.js') },
};

const { EV } = await makeScenario(
Expand Down Expand Up @@ -433,7 +420,7 @@ test('upgrade vat-bank', async t => {
test('upgrade vat-priceAuthority', async t => {
const bundles = {
priceAuthority: {
sourceSpec: importSpec('@agoric/vats/src/vat-priceAuthority.js'),
sourceSpec: resolvePath('@agoric/vats/src/vat-priceAuthority.js'),
},
};

Expand Down
4 changes: 2 additions & 2 deletions packages/builders/scripts/inter-protocol/deploy-contracts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env node
import { makeHelpers } from '@agoric/deploy-script-support';
import { E } from '@endo/eventual-send';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { resolvePath } from '@agoric/swingset-vat/tools/paths.js';
import { getCopyMapEntries, makeCopyMap } from '@agoric/store';

// TODO: CLI options to choose contracts
Expand All @@ -15,7 +15,7 @@ const contractRefs = [
'../bundles/bundle-auctioneer.js',
'../../vats/bundles/bundle-mintHolder.js',
];
const contractRoots = contractRefs.map(pkgAbsPath);
const contractRoots = contractRefs.map(resolvePath);

/** @type {<T>(store: any, key: string, make: () => T) => Promise<T>} */
const provideWhen = async (store, key, make) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/builders/test/test-inter-proposals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @ts-check
import { test as anyTest } from '@agoric/zoe/tools/prepare-test-env-ava.js';
import * as ambientFs from 'fs';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { resolvePath } from '@agoric/swingset-vat/tools/paths.js';

const configSpecifier = '@agoric/vm-config/decentral-itest-vaults-config.json';
const noop = harden(() => {});
Expand All @@ -13,7 +13,7 @@ const test = anyTest;
const makeTestContext = t => {
/** @param {string} specifier */
const loadConfig = async specifier => {
const fullPath = pkgAbsPath(specifier);
const fullPath = resolvePath(specifier, import.meta.url);
t.is(typeof fullPath, 'string');
const txt = await ambientFs.promises.readFile(fullPath, 'utf-8');
t.is(typeof txt, 'string');
Expand Down
6 changes: 4 additions & 2 deletions packages/cosmic-swingset/src/chain-main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { makeShutdown } from '@agoric/internal/src/node/shutdown.js';
import * as STORAGE_PATH from '@agoric/internal/src/chain-storage-paths.js';
import * as ActionType from '@agoric/internal/src/action-types.js';
import { BridgeId as BRIDGE_ID } from '@agoric/internal';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { makeResolvePath } from '@agoric/swingset-vat/tools/paths.js';
import {
makeBufferedStorage,
makeReadCachingStorage,
Expand All @@ -48,6 +48,8 @@ import {
validateImporterOptions,
} from './import-kernel-db.js';

const resolvePath = makeResolvePath(import.meta.url);

// eslint-disable-next-line no-unused-vars
let whenHellFreezesOver = null;

Expand Down Expand Up @@ -360,7 +362,7 @@ export default async function main(progname, args, { env, homedir, agcc }) {
bootMsg,
};
const getVatConfig = () =>
pkgAbsPath(
resolvePath(
env.CHAIN_BOOTSTRAP_VAT_CONFIG ||
argv.bootMsg.params.bootstrap_vat_config,
);
Expand Down
4 changes: 2 additions & 2 deletions packages/cosmic-swingset/src/sim-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { makeSlogSender } from '@agoric/telemetry';
import { Fail } from '@agoric/assert';
import { makeWithQueue } from '@agoric/internal/src/queue.js';
import { makeBatchedDeliver } from '@agoric/internal/src/batched-deliver.js';
import { pkgAbsPath } from '@agoric/swingset-vat/tools/paths.js';
import { resolvePath } from '@agoric/swingset-vat/tools/paths.js';
import stringify from './helpers/json-stable-stringify.js';
import { launch } from './launch-chain.js';
import { getTelemetryProviders } from './kernel-stats.js';
Expand Down Expand Up @@ -75,7 +75,7 @@ export async function connectToFakeChain(basedir, GCI, delay, inbound) {
};

const getVatConfig = () =>
pkgAbsPath(
resolvePath(
process.env.CHAIN_BOOTSTRAP_VAT_CONFIG ||
argv.bootMsg.params.bootstrap_vat_config,
);
Expand Down
Loading