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

Use node package resolution for bundle sources, enable Zoe in cosmic-swingset to use prebundled zcf #1462

Merged
merged 3 commits into from
Aug 13, 2020
Merged

Use node package resolution for bundle sources, enable Zoe in cosmic-swingset to use prebundled zcf #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c02653
feat: support use of module references in swingset config sourceSpecs
FUDCo Aug 13, 2020
2859650
feat: make Zoe in cosmic-swingset work with prebundled zcf
FUDCo Aug 13, 2020
e979475
fix: handle relative paths more better
FUDCo Aug 13, 2020
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
63 changes: 47 additions & 16 deletions packages/SwingSet/src/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,47 @@ export function loadBasedir(basedir) {
return config;
}

/**
* Resolve a pathname found in a config descriptor. First try to resolve it as
* a module path, and then if that doesn't work try to resolve it as an
* ordinary path relative to the directory in which the config file was found.
*
* @param dirname Path to directory containing the config file
* @param specPath Path found in a `sourceSpec` or `bundleSpec` property
*
* @return the absolute path corresponding to `specPath` if it can be
* determined.
*/
function resolveSpecFromConfig(dirname, specPath) {
try {
return require.resolve(specPath, { path: [dirname] });
} catch (e) {
if (e.code !== 'MODULE_NOT_FOUND') {
throw e;
}
}
return path.resolve(dirname, specPath);
Copy link
Member

Choose a reason for hiding this comment

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

Okay, now I see what you're going for. I, personally, would prefer that a specifier is the exact same resolution semantics as for modules (i.e. use ./foo.js if you mean foo.js in the current directory). If you agree with that sentiment, you could print a deprecation warning if the path.resolve is hit.

Up to you, though. I can see the convenience of being able to resolve bare specifiers relative to dirname, even though it gives me the willies. 😄

Copy link
Member

@michaelfig michaelfig Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Would it maybe be better to try path.resolve() first on bare specifiers, then punt to require.resolve for everything else or if the resolve doesn't result in a file? That would eliminate the surprise when you do "lodash" but you meant "./lodash/index.js".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind that these paths appear inside config files and have a different use pattern from imports. With imports, it's common that a large fraction of the stuff you are referring to comes from someplace else. In config files, non-local references are the exception; you would never reference "./lodash/index.js". Not having to stick a redundant ./ in front of almost literally everything improves readability quite a bit.

}

/**
* For each entry in a config descriptor (i.e, `vats`, `bundles`, etc), convert
* it to normal form: resolve each pathname to a context-insensitive absolute
* path and make sure it has a `parameters` property if it's supposed to.
*
* @param desc The config descriptor to be normalized.
* @param dirname The pathname of the directory in which the config file was found
* @param expectParameters `true` if the entries should have parameters (for
* example, `true` for `vats` but `false` for bundles).
*/
function normalizeConfigDescriptor(desc, dirname, expectParameters) {
if (desc) {
for (const name of Object.keys(desc)) {
const entry = desc[name];
if (entry.sourceSpec) {
entry.sourceSpec = path.resolve(dirname, entry.sourceSpec);
entry.sourceSpec = resolveSpecFromConfig(dirname, entry.sourceSpec);
}
if (entry.bundleSpec) {
entry.bundleSpec = path.resolve(dirname, entry.bundleSpec);
entry.bundleSpec = resolveSpecFromConfig(dirname, entry.bundleSpec);
}
if (expectParameters && !entry.parameters) {
entry.parameters = {};
Expand All @@ -142,6 +174,17 @@ function normalizeConfigDescriptor(desc, dirname, expectParameters) {
}
}

/**
* Read and parse a swingset config file and return it in normalized form.
*
* @param configPath Path to the config file to be processed
*
* @return the contained config object, in normalized form, or null if the
* requested config file did not exist.
*
* @throws if the file existed but was inaccessible, malformed, or otherwise
* invalid.
*/
export function loadSwingsetConfigFile(configPath) {
try {
const config = JSON.parse(fs.readFileSync(configPath));
Expand Down Expand Up @@ -398,21 +441,9 @@ export async function buildVatController(
if (!desc.bundleName) {
names.push(name);
if (desc.sourceSpec) {
const sourceSpec = desc.sourceSpec;
if (!(sourceSpec[0] === '.' || path.isAbsolute(sourceSpec))) {
throw Error(
'sourceSpec must be relative (./foo) or absolute (/foo) not bare (foo)',
);
}
presumptiveBundles.push(bundleSource(sourceSpec));
presumptiveBundles.push(bundleSource(desc.sourceSpec));
} else if (desc.bundleSpec) {
const bundleSpec = desc.bundleSpec;
if (!(bundleSpec[0] === '.' || path.isAbsolute(bundleSpec))) {
throw Error(
'bundleSpec must be relative (./foo) or absolute (/foo) not bare (foo)',
);
}
presumptiveBundles.push(fs.readFileSync(bundleSpec));
presumptiveBundles.push(fs.readFileSync(desc.bundleSpec));
} else if (desc.bundle) {
presumptiveBundles.push(desc.bundle);
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/cosmic-swingset/lib/ag-solo/init-basedir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export default function initBasedir(
const dstVatdir = path.join(basedir, 'vats');
fs.mkdirSync(dstVatdir);
fs.readdirSync(srcVatdir)
.filter(name => name.match(/\.js$/))
.filter(name => name.match(/\.(js|json)$/))
.forEach(name => {
fs.copyFileSync(path.join(srcVatdir, name), path.join(dstVatdir, name));
});
Expand Down
2 changes: 1 addition & 1 deletion packages/cosmic-swingset/lib/ag-solo/vats/chain-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"bootstrap": "bootstrap",
"bundles": {
"zcf": {
"sourceSpec": "../../../../zoe/src/contractFacet.js"
"sourceSpec": "@agoric/zoe/src/contractFacet"
}
},
"vats": {
Expand Down
2 changes: 1 addition & 1 deletion packages/cosmic-swingset/lib/ag-solo/vats/solo-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"bootstrap": "bootstrap",
"bundles": {
"zcf": {
"sourceSpec": "../../../../zoe/src/contractFacet.js"
"sourceSpec": "@agoric/zoe/src/contractFacet"
}
},
"vats": {
Expand Down
4 changes: 2 additions & 2 deletions packages/cosmic-swingset/lib/ag-solo/vats/vat-zoe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

import { makeZoe } from '@agoric/zoe';

export function buildRootObject(_vatPowers) {
export function buildRootObject(_vatPowers, vatParameters) {
return harden({
buildZoe: adminVat => makeZoe(adminVat),
buildZoe: adminVat => makeZoe(adminVat, vatParameters.zcfBundleName),
});
}
2 changes: 1 addition & 1 deletion packages/swingset-runner/demo/exchangeBenchmark/swingset.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"bootstrap": "bootstrap",
"bundles": {
"zcf": {
"sourceSpec": "../../../zoe/src/contractFacet.js"
"sourceSpec": "@agoric/zoe/src/contractFacet"
}
},
"vats": {
Expand Down