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: new include_elf! macro for importing ELF #1620

Merged

Conversation

xJonathanLEI
Copy link
Contributor

Adds a new mechanism of importing ELF files without breaking the existing ELF-copying behaviour. Note that this backward-compatibility is preserved only when building a single target, which is fine as building multiple binary targets doesn't even work at the moment in the first place.

A follow-up PR should add an option to turn off the ELF-copying behaviour.

Copy link

github-actions bot commented Oct 8, 2024

SP1 Performance Test Results

Branch: jon/gro-155-use-elf-from-target-instead-of-copying
Commit: d4321b57
Author: xJonathanLEI

program cycles execute (mHz) core (kHZ) compress (KHz) time success
fibonacci 11291 0.18 6.32 0.16 1m11s
ssz-withdrawals 2757356 17.28 62.92 17.38 2m39s
tendermint 12593597 6.57 167.34 63.64 3m20s

@xJonathanLEI xJonathanLEI enabled auto-merge (squash) October 8, 2024 15:48
Copy link
Contributor

@puma314 puma314 left a comment

Choose a reason for hiding this comment

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

Can we add an example for this to make sure that it works? (Or modify an existing example)?

@xJonathanLEI
Copy link
Contributor Author

Sure I can make all existing examples use this new syntax.

crates/build/src/build.rs Show resolved Hide resolved
crates/build/src/build.rs Show resolved Hide resolved
.find(|p| &p.id == program_crate)
.ok_or_else(|| anyhow::anyhow!("cannot find package for {}", program_crate))?;

for bin_target in program.targets.iter().filter(|t| {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we ignore --bin targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Exactly the opposite. We only care about bin targets.

In terms of the --bin flag, this is currently arlready supported as an option before this PR. When that flag is used it only builds one target specified by it. So what's done here is just to preserve that behavior.

@xJonathanLEI xJonathanLEI force-pushed the jon/gro-155-use-elf-from-target-instead-of-copying branch from dc566a3 to 016b8d1 Compare October 8, 2024 17:39
@xJonathanLEI xJonathanLEI merged commit 9987854 into dev Oct 9, 2024
22 of 23 checks passed
@xJonathanLEI xJonathanLEI deleted the jon/gro-155-use-elf-from-target-instead-of-copying branch October 9, 2024 17:47
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.

3 participants