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

Refactor: Make it easier to compile gitoxide as dynlib #1384

Merged

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented May 27, 2024

And also optimize compilation time: By making plumbing and porcelain as modules the lib.rs, they can be compiled after the rmeta for the dependencies are generated.

For the uni.rs which uses both plumbing and porcelain, this would avoid compiling these two modules twice.

This PR would also help @holzschu 's use case.

With this PR merged and published to https://crates.io , you can compile gitoxide as a dynlib using:

Cargo.toml:

[profile]
name = "gitoxide-dynlib"
publish = false

[lib]
crate-type = ["cdylib"]
bench = false

[dependencies.anyhow]
version = "1.0.86"

[dependencies.gitoxide]
version = "0.36.0"  # Or you could use git = "https://github.com/Byron/gitoxide"
features = ["max-pure"]

in src/lib.rs:

use anyhow::Result;

#[no_mangle]
pub extern "C" fn gix_main() -> Result<()> {
    gitoxide::plumbing::main()
}

#[no_mangle]
pub extern "C" fn ein_main() -> Result<()> {
    gitoxide::porcelain::main()
}

Then you would have a C dynlib, with two C API in it: gix_main and ein_main

And also optimize compilation time: By making `plumbing` and `porcelain`
as modules the `lib.rs`, they can be compiled after the rmeta for the
dependencies are generated.

For the `uni.rs` which uses both `plumbing` and `porcelain`, this would avoid
compiling these two modules twice.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Contributor Author

@Byron It seems like the CI is failing because of use of unsafe in gitoxide conflicts with forbid(unsafe)

@Byron
Copy link
Member

Byron commented May 27, 2024

Thanks a lot for your help!

Regarding CI, if this happens, I usually put the lib-attribute to deny. Will take a look later if you don't fix it before me.

Lastly:

use anyhow::Result;

#[no_mangle]
pub extern "C" fn gix_main() -> Result<()> {
    crate::plumbing::main()
}

#[no_mangle]
pub extern "C" fn ein_main() -> Result<()> {
    crate::porcelain::main()
}

Should that not be gitixoide::porcelain::main()?

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Contributor Author

Thanks, I've fixed the compilation error.

Should that not be gitixoide::porcelain::main()?

Turns out that I also uses the wrong path in this PR, update the description and the PR.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Looks great now, thank you!

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Contributor Author

Just found another compilation error.

And I've run cargo build --release --timings:

image

Moving these module plumbing and porcelain to lib.rs enables these two modules to compile in parallel to gitoxide-core

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Jiahao XU <[email protected]>
@Byron Byron merged commit bb30e52 into GitoxideLabs:main May 27, 2024
19 checks passed
@NobodyXu NobodyXu deleted the feat/easier-to-compile-gix-as-dynlib branch May 28, 2024 00:36
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