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

Adds re-exporting macros and crate renaming support; #57

Closed
wants to merge 5 commits into from

Conversation

dman-os
Copy link

@dman-os dman-os commented Mar 16, 2021

The scenario:

I want to pull all my dependencies through a deps crate in my workspace and dynamic link to this deps crate to speed up compilation allowing one fast iteration times.

Look at the strategy deployed by bevy for an example.

The challenge:

Proc macro's stop working as they expect to find some of the crates they depend on to be imported in the current crate.

The bytemuck_derive crate works with types from bytemuck using snippets as the one given below...

    quote!(::bytemuck::Pod)

...which fails to find the bytemuck crate as it's located in crate::deps::bytemuck.

Similar issues elsewhere: serde, yew

The solution:

If unable to find bytemuck crate in the manifest of the crate using the macro (detection using proc-macro-crate), use self::bytemuck qualifier. Anyone who intends to use re-exporting can then import bytemuck::self locally.

use deps::bytemuck{self, Zeroable, Pod};

#[repr(C)]
#[derive(Clone, Copy, Zeroable, Pod)]
struct Vertex{
    pos: [u32; 4],
}

We avoid breaking changes by using the old ::bytemuck qualifier if bytemuck is found in the manifest.

Courtesy of proc-macro-crate, we're also able to handle if bytemuck is found in the manifest but under a different name.

Includes:

  • adds bytemuck_derive into a workspace rooted in bytemuck

Inludes:
- adds bytemuck_derive into a workspace rooted in bytemuck
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Changes are sensible 👍 but for two things (c.f. the attached comments):

  • The proc-macro-crate dependency should be feature-gated (I'll let @Lokathor judge whether that should be an enabled-by-default feature or not; I think it's better for it not to be enabled by default), since it may break builds for #![no_std] dependencies;

  • Using self::bytemuck in the no-explicit-and-direct-bytemuck-dependency case seems unnecessarily restrictive: using bytemuck can be just as useful, with the added benefit that if bytemuck happens to be an extern crate, then users won't need to write use ::bytemuck /* ::{self} */;

derive/src/traits.rs Outdated Show resolved Hide resolved
derive/src/traits.rs Show resolved Hide resolved
derive/Cargo.toml Outdated Show resolved Hide resolved
derive/src/traits.rs Outdated Show resolved Hide resolved
dman-os and others added 3 commits March 17, 2021 04:41
Includes:
- Add feature gate for additions and `proc-macro-crate` crate to maintain `no-std` support
- Use  `Span::mixed_site` on case where unable to locate `bytemuck` in manifest to improve flexibility

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>
@dman-os
Copy link
Author

dman-os commented Mar 17, 2021

@danielhenrymantilla good suggestions. They've been applied.

@Lokathor
Copy link
Owner

You know that generic functions are code-generated in the crate that uses them, right? So if you use any generic things, including all of bytemuck, they'll still get put into your own crate and code generated there.

@dman-os
Copy link
Author

dman-os commented Mar 27, 2021

@Lokathor okaayy. Sorry, I'm new to rust and...what exactly does that change?

@Lokathor
Copy link
Owner

Basically I'm very skeptical that this "deps crate" build style will help your usage of generic functions. When you build the deps crate the dylib literally does not contain actual code for generic functions. Generics are monomorphized in the crate that uses them, so generic code is only actually generated when building a crate that uses the generic function. You can't just re-export bytemuck::cast in a project_deps crate to get the cast function to build into the dylib and cheaply link to that.

I'm sure you get some build time benefit overall, but bytemuck specifically, being 99% generic and heavily inlined, wouldn't see any gains.

@Lokathor
Copy link
Owner

Which is not to say that we can't merge the PR (I'll try to look closely at it Sun/Mon), but you should be aware that this might add up to "no difference at all"

@dman-os
Copy link
Author

dman-os commented Mar 28, 2021

build style will help your usage of generic functions
being 99% generic
Generics are monomorphized in the crate that uses them

OHh. Okay. Damn. I see why rustaceans avoid generics like the plague. I'm guessing they don't benefit from any incremental compilations.

Anyways, I guess I could choose and bring in some dependencies into the main crate. Having them all in one crate would be nice but I'm playing with a toy project and I could just work off my fork.

Thanks for your time.

@Lokathor
Copy link
Owner

They do get some incremental support, but they're not a "build and never touch it again" sort of situation like you get with C libs.

@Lokathor
Copy link
Owner

Lokathor commented Jun 7, 2021

Since this PR can't even really accomplish the initial goal (less rebuild times), and since I'm not usually a workspace user and I'd have to learn about them if one was added to this project, I'm gonna close the PR.

@Lokathor Lokathor closed this Jun 7, 2021
@blueforesticarus
Copy link

I think the primary utility of this is the ability to reexport proc macros in a "common" crate, which usefull in a workspace repos where the alternative is to copy the dependency to 20 Cargo.tomls

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.

4 participants