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

feat(cli): forge build for tests by default #1473

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

feat(cli): forge build for tests by default #1473

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5392c68
improve error for cannon test in ci
noahlitvin Oct 6, 2024
81e4ce5
rebuild by default
noahlitvin Oct 10, 2024
180a913
explicit ast
noahlitvin Oct 10, 2024
e492884
addressed feedback
noahlitvin Oct 16, 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
4 changes: 4 additions & 0 deletions packages/cli/src/commands/config/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,10 @@ export const commandsConfig: CommandsConfig = {
description: 'Use an alternative forge call, such as "coverage"',
defaultValue: 'test',
},
{
flags: '--skip-compile',
description: 'Skip the compilation step and use the existing artifacts',
},
...debugVerbosity,
],
forgeOptions: forgeTestOptions,
Expand Down
51 changes: 48 additions & 3 deletions packages/cli/src/foundry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { spawn } from 'node:child_process';
import path from 'path';
import fs from 'fs-extra';
import { glob } from 'glob';
import { ContractArtifact } from '@usecannon/builder';
import _ from 'lodash';
import Debug from 'debug';
import Debug, { log } from 'debug';

import { execPromise } from './helpers';
import { warn } from './util/console';
import { bold, gray, red, yellow } from 'chalk';
import { forgeBuildOptions } from './commands/config/forge/build';
import { fromFoundryOptionsToArgs, pickForgeBuildOptions } from './util/foundry-options';

const debug = Debug('cannon:cli:foundry');

Expand Down Expand Up @@ -72,11 +76,15 @@ export async function getFoundryArtifact(name: string, baseDir = '', includeSour
const possibleArtifacts = [];
for (const artifactPath of possibleArtifactPaths) {
const artifactBuffer = await fs.readFile(artifactPath);
possibleArtifacts.push(JSON.parse(artifactBuffer.toString()) as any);
const artifact = JSON.parse(artifactBuffer.toString()) as any;
if (!artifact.ast) {
throw new Error(`Unable to find output from forge with ast for ${inputContractName} (from ${name}). Before running this command, run forge build --ast`);
}
possibleArtifacts.push(artifact);
}

if (!possibleArtifacts.length) {
throw new Error(`no contract was found by name: ${inputContractName} (from ${name})`);
throw new Error(`Unable to find output from forge with ast for ${inputContractName} (from ${name}). Before running this command, run forge build --ast`);
Comment on lines +79 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

The cannon test command now runs forgeBuild, ensuring that artifacts always include the AST. This fix resolves the issue of missing AST in artifacts, so theoretically, this should no longer occur.

Previously, cannon test didn’t trigger a forge build, so forge executes forge build (when it wasn't built or it was the first time like for instance the CI) behind the scenes and by default it does not include the AST.

}

let artifact = possibleArtifacts[0];
Expand Down Expand Up @@ -153,3 +161,40 @@ export async function getFoundryArtifact(name: string, baseDir = '', includeSour
linkReferences: artifact.bytecode.linkReferences,
};
}


export async function forgeBuild(options: Record<string, any>, cannonfile: string) {

const cannonfilePath = path.resolve(cannonfile);
const projectDirectory = path.dirname(cannonfilePath);

log(bold('Building the foundry project...'));
Copy link

Choose a reason for hiding this comment

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

This could be inside line 172

if (!options.skipCompile) {
// use --build-info to output build info
// ref: https://github.com/foundry-rs/foundry/pull/7197
const forgeBuildArgs = [
'build',
'--build-info',
'--ast',
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the --ast flag.

Context: In a past Forge update (commit #7197) stopped generating AST by default. This creates compatibility issues with older Forge versions, where the AST was generated by default, and using the --ast flag can now trigger errors since some versions don’t recognize it.

Instead, the --build-info flag is supported across all Forge versions and reliably outputs the AST, making it the safer option.

...fromFoundryOptionsToArgs(pickForgeBuildOptions(options), forgeBuildOptions),
];

const forgeBuildProcess = spawn('forge', forgeBuildArgs, { cwd: projectDirectory, shell: true });

await new Promise((resolve, reject) => {
forgeBuildProcess.on('exit', (code) => {
if (code === 0) {
log(gray('forge build succeeded'));
} else {
log(red('forge build failed'));
log(red('Make sure "forge build" runs successfully or use the --skip-compile flag.'));
return reject(new Error(`forge build failed with exit code "${code}"`));
}

resolve(null);
});
});
} else {
log(yellow('Skipping forge build...'));
Copy link

@juanpernu juanpernu Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here could be something like: log(yellow('--skip-compile flag detected, skipping forge build...'));?🤔

}
}
37 changes: 5 additions & 32 deletions packages/cli/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import { writeModuleDeployments } from './util/write-deployments';
import './custom-steps/run';
import { ANVIL_PORT_DEFAULT_VALUE } from './constants';
import { deprecatedWarn } from './util/deprecated-warn';
import { forgeBuild } from './foundry';

export * from './types';
export * from './constants';
Expand Down Expand Up @@ -167,42 +168,12 @@ applyCommandsConfig(program.command('build'), commandsConfig.build)
options.port = options['anvil.port'];
}

const cannonfilePath = path.resolve(cannonfile);
const projectDirectory = path.dirname(cannonfilePath);

const cliSettings = resolveCliSettings(options);

// throw an error if the chainId is not consistent with the provider's chainId
await ensureChainIdConsistency(cliSettings.rpcUrl, options.chainId);

log(bold('Building the foundry project...'));
if (!options.skipCompile) {
// use --build-info to output build info
// ref: https://github.com/foundry-rs/foundry/pull/7197
const forgeBuildArgs = [
'build',
'--build-info',
...fromFoundryOptionsToArgs(pickForgeBuildOptions(options), forgeBuildOptions),
];

const forgeBuildProcess = spawn('forge', forgeBuildArgs, { cwd: projectDirectory, shell: true });

await new Promise((resolve, reject) => {
forgeBuildProcess.on('exit', (code) => {
if (code === 0) {
log(gray('forge build succeeded'));
} else {
log(red('forge build failed'));
log(red('Make sure "forge build" runs successfully or use the --skip-compile flag.'));
return reject(new Error(`forge build failed with exit code "${code}"`));
}

resolve(null);
});
});
} else {
log(yellow('Skipping forge build...'));
}
await forgeBuild(options, cannonfile);

log(''); // Linebreak in CLI to signify end of compilation.

Expand Down Expand Up @@ -666,6 +637,8 @@ applyCommandsConfig(program.command('test'), commandsConfig.test).action(async f
// throw an error if the chainId is not consistent with the provider's chainId
await ensureChainIdConsistency(cliSettings.rpcUrl, options.chainId);

await forgeBuild(options, cannonfile);

const [node, , outputs] = await doBuild(cannonfile, [], options);

// basically we need to write deployments here
Expand Down Expand Up @@ -805,4 +778,4 @@ applyCommandsConfig(pluginCmd.command('remove'), commandsConfig.plugin.commands.
log('Complete!');
});

export default program;
export default program;
Loading