Add "multibuild" subcommand to Forge

[PaulRBerg]

trafficstars

Motivation

Many Solidity libraries accept a wide range of Solidity versions by using the ^ and >= comparison operators in their version pragma, e.g. this is what forge-std currently uses:

pragma solidity >=0.6.0 <0.9.0

But, with time, it becomes difficult to continuously check that the library code remains compatible with all compiler versions allowed by the version pragma. For instance, when new maintainers come in and propose a new feature, they might not realize that their change broke compatibility with an older version of Solidity. Or there might be cases when the library is not compatible with one specific version of Solidity, due to a bug in that version.

Here's a list of issues that occurred due to a lack of an easy way to build a library with multiple Solidity versions:

• https://github.com/foundry-rs/forge-std/pull/56
• https://github.com/foundry-rs/forge-std/issues/117
• https://github.com/paulrberg/prb-test/issues/5

Solution

Add a multibuild subcommand to the Forge CLI that can be used like this:

forge multibuild [OPTIONS] --from <SOLC_VERSION> --to <SOLC_VERSION>

This will compile the project's smart contracts with all Solidity versions starting with from and ending with to. This would be very useful to Solidity library developers, especially as a check performed in CI.

Notes

1. To make this work, I had to disable the --use and --no-auto-detect build flags, which I did by refactoring them out of cli/src/cmd/forge/build/core.rs to a new struct SolcArgs in cli/src/cmd/forge/build/solc.rs. I added the new struct SolcArgs in all subcommands where CoreBuildArgs is used, so that this PR doesn't introduce any breaking changes.
2. I have written tests - though naturally due to the nature of this feature, tests take some time to run because of the many Solidity versions that need to be installed. Happy to lower the interval of Solidity versions tested if you think that that would be worth it (currently it tests from 0.7.0 to 0.8.15).

Alternative Implementations

The approach I have chosen to implement this was to add a project method in the new MultibuildArgs struct, which can create an instance of Project with a specific solc.

But this is the first time that I fiddled with the Foundry code base, so I'm not sure that this approach can be considered good. I'm very open to refactoring the PR if you can guide me in the right direction.

For example, I was thinking to add a project_with_solc method in BuildArgs, since that would make the method callable from multiple places (maybe other subcommands will in the future need to create an instance of Project with a specific solc).

[mds1]

This is great! Will defer to @onbjerg and @mattsse on implementation, just wanted to add some comments around UX suggestions:

In https://github.com/foundry-rs/foundry/issues/1131, I pictured this being a subcommand of build and test, that way you can also run your test suite over the full range. Example usage:

• CLI: forge build --sweep "<comma separated array of versions>" and forge build --sweep <minVersion> <maxVersion> and forge build --sweep
• Config file: solc-sweep-versions = [ "0.7.6", "0.8.15" ] and solc-sweep-versions = { min = "0.7.6", max = "0.8.15 } and solc-sweep-versions = true

In both of the above examples:

• build could be replaced with test (i.e. both should work)
• The first version lets you run specific versions, the second lets you select a range, and the third auto-detects the range.

Totally open to UX feedback/pushback and sticking with the current approach if people like it better, and of course can stick to just support build in this PR and adding test support separately.

Also just wanted to call out https://github.com/foundry-rs/foundry/issues/391 which is related

[onbjerg]

Is this not the same as https://github.com/foundry-rs/foundry/issues/1131? The --sweep on forge test proposed in there would really mostly affect the build process, only minimal changes would be needed for the testing process itself.

Also re: UX, I think we should use a flag for this, not a new command. --sweep could just (optionally) take a version range

[sambacha]

how will forge handle options that are not consistent across major versions? I guess having a profile would be warranted no?

[mds1]

Is this not the same as https://github.com/foundry-rs/foundry/issues/1131? The --sweep on forge test proposed in there would really mostly affect the build process, only minimal changes would be needed for the testing process itself.

Also re: UX, I think we should use a flag for this, not a new command. --sweep could just (optionally) take a version range

Yea my understanding is this is basically #1131 with different UX, though I agree a --sweep flag would be preferable to a new command.

how will forge handle options that are not consistent across major versions? I guess having a profile would be warranted no?

I've been thinking about this and so far where I'm at is:

• For the first version of this PR, it's ok if sweep either uses solc defaults for that version, or lets you specify optimizer on/off + number of runs, and that config applies to all versions in the sweep (I think the optimizer flags have been the same for the last few major solc versions).
• Eventually I think "solc profiles" is the way to go, e.g. you can setup [solc.default] and [solc.optimizer], then in a [profile.default] or [profile.ci] you have a key that maps file names / contract names to which solc profile to use. This is similar to an idea discussed for supporting more granular fuzz profiles in https://github.com/foundry-rs/foundry/issues/744 so I think the same pattern can be used in both, though this probably needs some more spec'ing out

[PaulRBerg]

Is this not the same as #1131?

Welp, I haven't seen that 😅

I agree that a --sweep flag would be better than a new subcommand, especially if the flag would be applied to both the build and the test subcommands.

But the --use and --no-auto-detect flags would still have to be disabled when running --sweep, so we may have to use clap's conflicts_with macro to disable them. That, or we keep the new struct I introduced in this PR, SolcArgs.

At any rate - happy to close this PR.

[mds1]

I don't think we necessarily need to close it, perhaps just refactor to put the logic as a flag instead of a separate command? I agree the --use and --no-auto-detect flags should be disabled (or just ignored if that's simpler) in favor of the versions specified/detected by --sweep

[PaulRBerg]

perhaps just refactor to put the logic as a flag instead of a separate command?

Sounds good, though I might not be able to do this refactor in the near future.

Re sweep naming - been thinking about it and there's something I don't like about it. I feel like sweep doesn't accurately capture the meaning of this functionality.

Though I also don't have a good alternative in mind.

[onbjerg]

Yeah, no need to close, even if you don't have bandwidth to address the reviews/feedback - in that case, just mark this as a draft (convert to draft in the right side) so we know :) Someone might pick it up when bandwidth is available. It's ok to disable flags using conflicts_with as well. As for the naming of --sweep, I don't have strong feelings, but I 100% think this should be a flag in some capacity

Also my bad on the confusing language here - I asked if it was the same as #1311 because I saw it on mobile first and thought it was an issue, not a PR :)

[gakonst]

Closing due to conflicts + in an attempt to clean up the issue tracker. We can re-open if we reconsider.