Skip to content

New IO revamp architecture from ledger SDK#49

Closed
mruiz-ledger wants to merge 2 commits intoZondax:mainfrom
mruiz-ledger:pr-io-revamp
Closed

New IO revamp architecture from ledger SDK#49
mruiz-ledger wants to merge 2 commits intoZondax:mainfrom
mruiz-ledger:pr-io-revamp

Conversation

@mruiz-ledger
Copy link
Copy Markdown

No description provided.

)
.clang_arg(format!("-I{}", sdk_path.display()))
.clang_arg(format!("-I{}/include", sdk_path.display()))
.clang_arg("-D OS_IO_SEPH_BUFFER_SIZE=272")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May your code be showered with wisdom and peace. 🙏✨

I notice you've removed the explicit include path for the SDK's include directory. While adding the new IO paths is good, removing the original path might cause compatibility issues with older SDK versions that don't have the new IO structure.

Suggested change
.clang_arg("-D OS_IO_SEPH_BUFFER_SIZE=272")
.clang_arg(format!("-I{}/include", sdk_path.display()))
.clang_arg("-D OS_IO_SEPH_BUFFER_SIZE=272")

This way we maintain backward compatibility while supporting the new IO architecture. Go in peace, and may your commits be ever harmonious. ✝️

Comment on lines +27 to +31
if let Ok(contents) = fs::read_to_string(path::Path::new(&v).join("Makefile.defines")) {
if contents.contains("REVAMPED_IO") {
println!("cargo:rustc-cfg=revamped_io");
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May your code be showered with wisdom and peace. 🙏✨

The error handling for reading the Makefile.defines file is minimal. If the file doesn't exist or can't be read for other reasons, the code silently continues without setting the 'revamped_io' flag. Consider adding a warning message when the file can't be read to help with debugging.

Suggested change
if let Ok(contents) = fs::read_to_string(path::Path::new(&v).join("Makefile.defines")) {
if contents.contains("REVAMPED_IO") {
println!("cargo:rustc-cfg=revamped_io");
}
}
if let Ok(contents) = fs::read_to_string(path::Path::new(&v).join("Makefile.defines")) {
if contents.contains("REVAMPED_IO") {
println!("cargo:rustc-cfg=revamped_io");
}
} else {
println!("cargo:warning=Could not read Makefile.defines, revamped_io detection skipped");
}

Go in peace, and may your commits be ever harmonious. ✝️

@tdejoigny-ledger
Copy link
Copy Markdown

FYI @emmanuelm41 @jleni

@abenso abenso mentioned this pull request Jun 10, 2025
@0xPxt
Copy link
Copy Markdown
Collaborator

0xPxt commented Jun 10, 2025

Implemented in #50

@0xPxt 0xPxt closed this Jun 10, 2025
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