Skip to content

Support different lib.crate-type - #227

Open
NickHu wants to merge 1 commit into
nix-community:masterfrom
NickHu:cratetype
Open

Support different lib.crate-type#227
NickHu wants to merge 1 commit into
nix-community:masterfrom
NickHu:cratetype

Conversation

@NickHu

@NickHu NickHu commented Mar 7, 2022

Copy link
Copy Markdown

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.

@nmattia nmattia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Few questions!

Comment thread lib.nix
Comment on lines +214 to +215
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! { loop {} }

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread lib.nix
# 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread lib.nix
# Avoid accidentally pulling `std` for no-std crates.
echo '#![no_std]' >src/lib.rs
cat <<EOF >src/lib.rs
#![no_std]

Copy link
Copy Markdown
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.

@nmattia nmattia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Comment thread lib.nix
Comment on lines +214 to +215
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! { loop {} }

Copy link
Copy Markdown
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.

Comment thread lib.nix
# 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
Copy Markdown
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
Copy Markdown

@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