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

rand_core 0.4.2 (backports) #853

Merged
merged 10 commits into from Aug 1, 2019
Merged

rand_core 0.4.2 (backports) #853

merged 10 commits into from Aug 1, 2019

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jul 27, 2019

As requested in #783.

Note @gnzlbg this does not include the links attribute we discussed yesterday (#850), because I don't believe this is the best fix.

@dhardy dhardy requested a review from vks July 27, 2019 10:33
@dhardy dhardy changed the base branch from master to core-0.4 July 27, 2019 10:33
@gnzlbg
Copy link

gnzlbg commented Jul 27, 2019

The links attribute is a super strong hammer. There are ways to relax it a bit, but those are super hacky. So it should only really be used if there is absolutely no way for two crates to ever be part of the same dependency graph correctly (e.g. when linking a C library).

@vks
Copy link
Collaborator

vks commented Jul 29, 2019

There is also 90501dc, but this would have to be backported to rand 0.6.

@vks
Copy link
Collaborator

vks commented Jul 29, 2019

Why are the CI tests not running for this branch? Otherwise it looks fine to me.

@dhardy
Copy link
Member Author

dhardy commented Jul 29, 2019

That may be because I originally opened it vs the wrong branch... think I'll have to close this and re-open.

@dhardy dhardy closed this Jul 29, 2019
@dhardy dhardy reopened this Jul 29, 2019
@vks
Copy link
Collaborator

vks commented Jul 30, 2019

The failure in the average crate is weird. The crate should not use the serde1 feature.

@dhardy
Copy link
Member Author

dhardy commented Jul 30, 2019

Yes indeed. It's clear from the macro expansion that the compiler is referring to features behind that feature flag:

10  | |      [cfg_attr (feature = "serde1" , derive (Serialize , Deserialize))] pub
    | |                                                          ^^^^^^^^^^^

It's clear from cargo tree --features=serde1,log that there is only a single dependency on average, which from the Cargo.toml never specifies any feature flags.

😕

@dhardy
Copy link
Member Author

dhardy commented Jul 31, 2019

The define_histogram macro is defined here. @vks macros generate source code, hence that cfg(feature = "serde1") check only happens when instantiated inside the uniformity example.

I think therefore your macro is incorrect. Maybe the easiest solution would be to put the serde1 cfg outside the macro definition, and list both versions.

Inside rand, the serde1 feature we just tested only passes this feature to sub-crates, but doesn't do anything itself (thus the required serde libs are not linked). So I guess the solution is we shouldn't test rand with this feature active.

@vks
Copy link
Collaborator

vks commented Jul 31, 2019

I think therefore your macro is incorrect. Maybe the easiest solution would be to put the serde1 cfg outside the macro definition, and list both versions.

I just published average 0.10.2, hopefully fixing this bug by conditionally defining different macros.

You workaround was not complete, it seems cargo test --all-features is still executed.

@dhardy
Copy link
Member Author

dhardy commented Aug 1, 2019

Thanks. Then I'll bump the required version of average since removing --all-features tests might have unintended consequences.

I'll be away for a few days, so if I don't get this published I go then hopefully you can.

@dhardy
Copy link
Member Author

dhardy commented Aug 1, 2019

Now the 1.22 builder fails. I'd prefer not to bump the Rustc version for this release, so I guess the other solution is needed.

This feature is not applicable to rand, but is causing
issues due to a bug in average.
@vks
Copy link
Collaborator

vks commented Aug 1, 2019

Now the 1.22 builder fails.

Sorry about that, I have been following the minimal Rust version of Rand. However, it seems that disabling the serde tests worked.

@dhardy dhardy merged commit c4b9390 into rust-random:core-0.4 Aug 1, 2019
@dhardy
Copy link
Member Author

dhardy commented Aug 1, 2019

I forgot, we already pushed (and yanked) 0.4.1. I just bumped the version to 0.4.2 on master but publishing fails:

Caused by:
  patch for `rand_core` in `https://github.com/rust-lang/crates.io-index` did not resolve to any crates. If this is unexpected, you may wish to consult: https://github.com/rust-lang/cargo/issues/4678

Unfortunately I don't have time to look into this now.

@dhardy dhardy changed the title rand_core 0.4.1 (backports) rand_core 0.4.2 (backports) Aug 6, 2019
@dhardy
Copy link
Member Author

dhardy commented Aug 6, 2019

Published

@dhardy dhardy deleted the core-0.4 branch August 6, 2019 07:45
@dhardy dhardy mentioned this pull request Sep 4, 2019
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.

None yet

6 participants