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

WIP: Convert to workspace (multi crate) #143

Closed
wants to merge 144 commits into from

Conversation

CyonAlexRDX
Copy link
Contributor

@CyonAlexRDX CyonAlexRDX commented May 20, 2024

Warning

Have not managed UniFFI to work :/ it seems very very hard and Mozilla is unable to help. I have not found a single open source project with multi-UniFFI-crates setup, so not sure it works. Apart from this being based on a non-merged PR branch. So we need to park it for now. Probably need to extract ALL UniFFI stuff into one single "outer" crate. Which is a very big undertaking. We probably should do work "limiting the UniFFI surface" first. Maybe it would be a good idea to try to merge this with the file structure intact, but converting the lib files to mod files and ignore all extra UniFFI toml and Cargo toml files.

Workspace

This PR introduces workspace - a multicrate setup instead of mono-crate.

Important

Note that we are using a fork of a fork of UniFFI - from a WIP non merge PR, my fork of Bens fork fixes a problem where CommonError could not be used in other crates outside of sargoncommon crate (here is PR from me into Bens fork). We really want PR 2087 in UniFFI to land and maybe should not merge this PR until it does.

I wish it was as easy as "split files into folders", but if you ever done this sort of work you know that one suddenly is forced to think of hierarchy of inter-crate-dependencies.

Note

The split done by this PR should be regarded as a naive, first pass. There are almost certain many more optimizations we can get by futher splitting large crates, especially large is Profile. We SHOULD split it into at least two crates: profile_models_v100 - which almost never change and can avoid being rebuilt! and profile_logic crate with logic which we might change more frequently.

sccache

Now that we use workspace we can use scache with more success! So you SHOULD do this:

brew install sccache

And you SHOULD have direnv installed already. Dont forget to direnv allow . when you stand in Sargon root for the first

Timings

Machine: Mac Studio M1 Max

Incremental

Make single change in a single top of the hierarchy file - sargon_os.rs

Note

Before: 8.42 seconds
After: 1.45 seconds 🚀 6x speedup

Clean build

Before each build I do cargo clean.

cargo build

Unchanged | 1min 40 sec

cargo build --release

Before: 4min 47 seconds
Before: 2min 38 seconds 🚀 near 2x speedup

Changes

No logical changes should have occured. The only non-trivial change apart from lots of "superficial traits" (to solve Rust orphan rule) I was forced to change decl_identified_vec_of macro to not simply declare a type alias pub type Accounts = IdentfiedVecOf<Account>, but instead use a newtype: pub struct Accounts(IdentfiedVecOf<Account>). The reason is if we use the decl_identified_vec_of macro from another crate than the crate in which IdentfiedVecOf is declared, we cannot impl traits from other crates.

@CyonAlexRDX CyonAlexRDX changed the title WIP WIP: Convert to workspace (multi crate) May 21, 2024
@CyonAlexRDX CyonAlexRDX marked this pull request as ready for review May 21, 2024 18:34
// use crate::prelude::*;

// #[macro_export]
// macro_rules! decl_secret_bytes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

Mnemonic::from_entropy_in(entropy, language)
}
// #[uniffi::export]
// pub fn new_mnemonic_generate_with_entropy(
Copy link
Contributor

Choose a reason for hiding this comment

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

Migrate

// let sut = new_mnemonic_generate_with_entropy(
// BIP39Entropy::EntropyOf16Bytes(Entropy16Bytes::new([0xff; 16])),
// BIP39Language::English,
// );
Copy link
Contributor

Choose a reason for hiding this comment

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

Reintroduce

timings/before.html Outdated Show resolved Hide resolved
timings/timings.html Outdated Show resolved Hide resolved
@CyonAlexRDX CyonAlexRDX marked this pull request as draft May 23, 2024 07:20
@Sajjon
Copy link
Contributor

Sajjon commented May 26, 2024

We might get this to work, with an unconventional hybrid approach! Which is to use workspace for all things Rust and Cargo, but skip using it for Build process and all things UniFFI. The idea is we will use a workspace setup by default, and used for:

  • development
  • testing

But for release and UniFFI bindgen, we change the structure ephemerally to NOT be workspace, approximate steps:

  • Resurrect a lib.rs in "crates"
  • Modify Cargo.toml -> removing "workspace" ref, pointed to "lib.rs" in "crates"
  • Remove all extra UniFFI.toml, *.udl
  • Change content of lib.rs files in "crates" (might require massaging those abit, introducing one level of indirection), need to remove the uniffi::include_scaffolding!
  • Rename all lib.rs in "crates" -> mod.rs

So we would introduce a Mother Of All build scripts which is use for cargo build, which performs the steps laid out above, but before doing so it checks git status is clean, runs the script and at the ends git resets - maybe there is a better way to ensure those changes are temporary and reverted?

Base automatically changed from ac/system_with_drivers to sargon_os June 28, 2024 13:16
Base automatically changed from sargon_os to main July 29, 2024 14:31
@CyonAlexRDX CyonAlexRDX mentioned this pull request Aug 5, 2024
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.

None yet

2 participants