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.

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

feat: multi-crate ELF building support #1750

Merged
merged 33 commits into from
Nov 15, 2024
Merged

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Nov 5, 2024

I renamed --binary to --binaries as it's more accurate now, but I'm not sure as it's a breaking change.

Also, maybe I can do this:

// TODO: In the future, change this to default to the package name. Will require updating
// docs and examples.
args.binary.clone()

Copy link

github-actions bot commented Nov 5, 2024

SP1 Performance Test Results

Branch: aurelien/gro-154-multi-crate-elf
Commit: 03fe828
Author: leruaa

program cycles execute (mHz) core (kHZ) compress (KHz) time success
fibonacci 11291 0.18 2.81 0.46 25s
ssz-withdrawals 2757356 17.07 124.93 34.75 1m20s
tendermint 12593597 6.67 264.06 99.88 2m8s

@leruaa leruaa marked this pull request as ready for review November 7, 2024 00:41
@leruaa leruaa changed the title feat: allow to specify multiple targets feat: multi-crate ELF building support Nov 7, 2024
Copy link
Contributor

@yuwen01 yuwen01 left a comment

Choose a reason for hiding this comment

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

Are test artifacts automatically built when running tests? Is it feasible to do this in CI now, or even with a build.rs in the test-artifacts crate?

crates/core/machine/src/riscv/shape.rs Show resolved Hide resolved
crates/core/machine/src/syscall/precompiles/README.md Outdated Show resolved Hide resolved
crates/core/machine/src/syscall/precompiles/README.md Outdated Show resolved Hide resolved
crates/test-artifacts/src/lib.rs Outdated Show resolved Hide resolved
.github/workflows/pr.yml Show resolved Hide resolved
@yuwen01 yuwen01 self-requested a review November 14, 2024 23:39
Copy link
Contributor

@yuwen01 yuwen01 left a comment

Choose a reason for hiding this comment

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

LGTM

just for my own understanding, you use --binaries when you need to build multiple elfs for multiple sp1 programs within the same package, and you use --packages when you need to build multiple packages?

Does that mean code like this is now deprecated (from RSP)?

use sp1_build::build_program;

fn main() {
    build_program("../client-eth");
    build_program("../client-op");
    build_program("../client-linea");
}

And you'd instead do something like

build_program_with_args("path_to_workspace_root", BuildArgs{packages=vec!["client-linea", "client-op", "client-eth"], ..Default}

This is better than the previous approach, because it's parallelized.

@leruaa
Copy link
Contributor Author

leruaa commented Nov 14, 2024

@yuwen01 Yes exactly. And if the 3 crates in your example above are the only ones in theirs workspace, you don't even neet to specify them as --packages, it will build everything. --packages is if you want to filter.

@leruaa leruaa merged commit c974b47 into dev Nov 15, 2024
15 checks passed
@leruaa leruaa deleted the aurelien/gro-154-multi-crate-elf branch November 15, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants