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

cleanup proposal output from extractor #9058

Merged
merged 2 commits into from
Mar 11, 2024
Merged

cleanup proposal output from extractor #9058

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ab79b7
feat: delete tmp dir after proposal build
turadg Mar 11, 2024
5520375
refactor: fewer bindings in proposal extractor
turadg Mar 11, 2024
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
42 changes: 23 additions & 19 deletions packages/boot/tools/supports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,13 @@ export const makeProposalExtractor = ({ childProcess, fs }: Powers) => {
const importSpec = spec =>
importMetaResolve(spec, import.meta.url).then(u => new URL(u).pathname);

const runPackageScript = async (outputDir, scriptPath, env) => {
const runPackageScript = (
outputDir: string,
scriptPath: string,
env: NodeJS.ProcessEnv,
) => {
console.info('running package script:', scriptPath);
const out = await childProcess.execFileSync('yarn', ['bin', 'agoric'], {
const out = childProcess.execFileSync('yarn', ['bin', 'agoric'], {
cwd: outputDir,
env,
});
Expand Down Expand Up @@ -150,30 +154,30 @@ export const makeProposalExtractor = ({ childProcess, fs }: Powers) => {
};

const buildAndExtract = async (builderPath: string) => {
const scriptEnv = Object.assign(Object.create(process.env));

const pkgPath = getPkgPath('builders');
const tmpDir = await fsAmbientPromises.mkdtemp(join(pkgPath, 'proposal-'));

const scriptPath = await importSpec(builderPath);
const tmpDir = await fsAmbientPromises.mkdtemp(
join(getPkgPath('builders'), 'proposal-'),
);

// XXX use '@agoric/inter-protocol'?
const out = await runPackageScript(tmpDir, scriptPath, scriptEnv);
const built = parseProposalParts(out.toString());
const built = parseProposalParts(
runPackageScript(
tmpDir,
await importSpec(builderPath),
process.env,
).toString(),
);

const loadAndRmPkgFile = async fileName => {
const filePath = join(tmpDir, fileName);
const content = await fs.readFile(filePath, 'utf8');
await fs.rm(filePath);
return content;
};
const loadPkgFile = fileName => fs.readFile(join(tmpDir, fileName), 'utf8');

const evalsP = Promise.all(
built.evals.map(async ({ permit, script }) => {
const [permits, code] = await Promise.all([
loadAndRmPkgFile(permit),
loadAndRmPkgFile(script),
loadPkgFile(permit),
loadPkgFile(script),
]);
// Fire and forget. There's a chance the Node process could terminate
// before the deletion completes. This is a minor inconvenience to clean
// up manually and not worth slowing down the test execution to prevent.
void fsAmbientPromises.rm(tmpDir, { recursive: true, force: true });
Copy link
Member

Choose a reason for hiding this comment

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

This is the only line I find a little suspect. Does voiding instead of awaiting this promise mean the script might exit before tmpDir is fully removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's not awaited, if the Node process reaches termination before it's done I suppose it would be aborted. That seems very unlikely but on the outside chance it's not worth slowing down the test on that file IO.

Is a comment to that effect sufficient or do you think it's worth awaiting?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go with the comment since that was the request

return { json_permits: permits, js_code: code } as CoreEvalSDKType;
}),
);
Expand Down
Loading