-
Notifications
You must be signed in to change notification settings - Fork 82
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
Allow crates with cfg(feature = "x") to work #50
Conversation
…ntered We need to check the list of resolvedDefaultFeatures when we encounter `cfg(feature = "x")` since there can be multiple features specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much!
Could you reverse the change of included empty resolvedDefaultFeatures? Otherwise looks great.
Thanks for taking care of updating the tests.
@@ -104,6 +104,7 @@ rec { | |||
}; | |||
features = { | |||
}; | |||
resolvedDefaultFeatures = [ ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer leaving these out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually necessary for the Rust changes to work, because those changes introduce a check for features in the resolved features list
if !first { | ||
result.push_str(" && "); | ||
} | ||
render(result, &expressions[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is nicer :)
|
||
# tokio-executor + tracing triggers the use of cfg(feature = "...") | ||
tokio-executor = "*" | ||
# FIXME: using default features causes 2 versions of lazy_static to be found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixed, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please hide empty resolvedDefaultFeatures again.
Closes #37