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

#[no_std] support #148

Merged
merged 11 commits into from
Mar 16, 2019
Merged

#[no_std] support #148

merged 11 commits into from
Mar 16, 2019

Conversation

ExpHP
Copy link
Collaborator

@ExpHP ExpHP commented Feb 4, 2019

Closes #147

The breaking changes are:

  • A change in the signature of two functions (monoid::combine_all, semigroup::combine_all_option) may break type inference
  • Adding default-enabled features std and validated may break client crates using default-features = false.

The toughest part is the tests. Several doctests were cfged out using the fact that doc comments are actually attributes.

It is crucial that frunk_derive does not depend on the std feature of frunk_core, or else we'd have to remove all usage of #[derive] from the doc tests in frunk_core. This is because it is impossible to disable features of frunk_core enabled by frunk_derive due to the circular dev-dependency. (features enabled by dev-dependencies are enabled in all builds, not just tests)

Thankfully, frunk_derive currently does not actually need to depend on frunk_core at all, so I removed the dependency.

@ExpHP
Copy link
Collaborator Author

ExpHP commented Feb 5, 2019

Haven't looked at the failure yet, but the CI testing should be done differently

https://users.rust-lang.org/t/how-to-make-a-test-guarantee-that-a-crate-was-built-with-no-std/24964

@ExpHP
Copy link
Collaborator Author

ExpHP commented Feb 5, 2019

I expanded the initial comment.

@TheVova
Copy link

TheVova commented Feb 5, 2019

Nice! I'll give this a test spin and look around to see if i can help fix some stuff as i find them :)

@ExpHP
Copy link
Collaborator Author

ExpHP commented Feb 26, 2019

I just remembered that I forgot about this.

IIRC the most recently merged PR was incompatible with this, so it will need a quick rebase. We can start working towards a v0.3.0.

@ExpHP
Copy link
Collaborator Author

ExpHP commented Feb 26, 2019

Oh, for crying out loud:

I am once again stumped how to make this work. The last frunk update added all this proc macro stuff which uses the proc-macro2 crate which does not appear to support #[no_std].

I think more specifically the issue is that we use proc-macro-hack, which requires the existence of a normal crate frunk_proc_macros that depends on proc-macro-hack, which depends on proc-macro2. Because frunk_proc_macros is not a proc-macro crate, cargo sees all of its dependencies as runtime dependencies of frunk. I think.

@ExpHP
Copy link
Collaborator Author

ExpHP commented Mar 15, 2019

I'd like to merge this soon because basically every single intervening PR has broken it. This means the next version will have to be a major version bump.

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

Overall LGTM, the Cargo feature-flagging looks reasonable to me (though I must admit I'm not super experienced there), and the Rust changes look fine to me as well.

//! assert_eq!(d_user.first_name, "Joe");
//! # }
//! ```
#![cfg_attr(feature = "std", doc = r#"
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting !

@Centril
Copy link
Collaborator

Centril commented Mar 16, 2019

@lloydmeta @ExpHP Btw I should note that my language & release team duties currently prevent me from devoting any time to this crate atm. So please don't feel the need to ask for my opinions. :) (I might have time in the future...)

@ExpHP ExpHP merged commit 3905014 into lloydmeta:master Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants