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

Support different lib.crate-type #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NickHu
Copy link

@NickHu NickHu commented Mar 7, 2022

This is particularly important for the case of WASM binaries, which are necessarily compiled with crate-type=cdylib. Without this, when compiling for CARGO_BUILD_TARGET=wasm32-unknown-unknown, the -deps derivation strips the [lib] section of the Cargo.toml, so only lib*.d and lib*.rlib files artifacts are generated under target/. Then when the main derivation is compiled, the [lib] section is not stripped, so it must recompile all the dependencies again for cdylib in order to generate a wasm binary.

Copy link
Collaborator

@nmattia nmattia left a comment

Choose a reason for hiding this comment

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

Few questions!

Comment on lines +214 to +215
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! { loop {} }
Copy link
Collaborator

Choose a reason for hiding this comment

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

what are these two lines for?

Copy link
Author

Choose a reason for hiding this comment

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

If you compile with crate-type=cdylib, and also use no_std, the compilation will fail because no panic handler has been provided (the bit that unwinds the stack trace, usually provided by the standard library). This is just a dummy stub to get compilation to succeed. See https://doc.rust-lang.org/nomicon/panic-handler.html for more details.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Added a code suggestion; worth having the clarification near the code.

@@ -174,7 +174,7 @@ rec
attrs =
# Since we pretend everything is a lib, we remove any mentions
# of binaries
removeAttrs cargotoml [ "bin" "example" "lib" "test" "bench" "default-run" ]
removeAttrs cargotoml [ "bin" "example" "test" "bench" "default-run" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if the project mentions a [lib] that isn't src/lib.rs? Won't cargo try to build that and, not finding any files, fail?

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, this change in particular is the one I'm not sure about. In order for WASM dependencies to be built, lib.crate-type needs to be retained. This was merely the quickest fix I could think of, I didn't really consider the other effects, and perhaps there's a better way to achieve the same goal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see! I believe this will break on cargo tomls that have lib = .... Can you give some example Cargo.tomls that use lib.crate-type? Maybe we can just remove e.g. the path of the lib attribute?

Copy link
Author

Choose a reason for hiding this comment

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

Any example which uses wasm-bindgen for instance:
https://github.com/rustwasm/wasm-bindgen/tree/main/examples

@@ -209,7 +209,11 @@ rec
pushd $out/$member > /dev/null
mkdir -p src
# Avoid accidentally pulling `std` for no-std crates.
echo '#![no_std]' >src/lib.rs
cat <<EOF >src/lib.rs
#![no_std]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#![no_std]
#![no_std]
# If you compile with crate-type=cdylib, and also use no_std,
# the compilation will fail because no panic handler has been
# provided (the bit that unwinds the stack trace, usually provided
# by the standard library). This is just a dummy stub to get
# compilation to succeed.
# See https://doc.rust-lang.org/nomicon/panic-handler.html for more details.

Copy link
Collaborator

@nmattia nmattia left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +214 to +215
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! { loop {} }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Added a code suggestion; worth having the clarification near the code.

@@ -174,7 +174,7 @@ rec
attrs =
# Since we pretend everything is a lib, we remove any mentions
# of binaries
removeAttrs cargotoml [ "bin" "example" "lib" "test" "bench" "default-run" ]
removeAttrs cargotoml [ "bin" "example" "test" "bench" "default-run" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see! I believe this will break on cargo tomls that have lib = .... Can you give some example Cargo.tomls that use lib.crate-type? Maybe we can just remove e.g. the path of the lib attribute?

@njaremko
Copy link

@NickHu Are you still working on this? If not, I'll pick it up because I need this working 😆

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