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

MSRV bump, module changes #15

Merged
merged 5 commits into from
Mar 23, 2019
Merged

MSRV bump, module changes #15

merged 5 commits into from
Mar 23, 2019

Conversation

newpavlov
Copy link
Member

Note that with this PR getrandom by default will not use anything from std anymore, so it will be possible to implement #4.

@newpavlov newpavlov requested a review from dhardy March 22, 2019 17:34
- rust: 1.28.0
env: DESCRIPTION="OSX, 1.22.0"
- rust: 1.32.0
env: DESCRIPTION="OSX, 1.32.0"
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to use 1.31 as the MSRV? Also you need to update the note in the README.

Copy link
Member Author

@newpavlov newpavlov Mar 22, 2019

Choose a reason for hiding this comment

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

extern crate std; and use std::<..>; in modules does not work on 1.31.0. It fails with "error[E0658]: imports can only refer to extern crate names passed with --extern on stable channel (see issue #53130)". I think we could fix it by moving extern crate std; to lib.rs, but it will add a sizable cfg attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. 1.32 is barely over two months old, but there doesn't appear to be much demand for back compatibility. Perhaps we should not do the same for the main Rand project yet though just in case we need to revert this.

@dhardy
Copy link
Member

dhardy commented Mar 22, 2019

error[E0468]: an `extern crate` loading macros must be at the crate root
  --> src/wasm32_stdweb.rs:11:14
   |
11 | #[macro_use] extern crate stdweb;
   |              ^^^^^^^^^^^^^^^^^^^^

Can we use Edition 2018 style macro imports?

@newpavlov
Copy link
Member Author

I looks like stdweb does not work nicely with 2018 imports. I think I've worked around it, but ideally I would like to remove stdweb support in favour of wasm-bindgen. IIUC the necessary work on bindgen side is already done.

@dhardy
Copy link
Member

dhardy commented Mar 23, 2019

Seems like your workaround does the job for now. It may require fixing again later, but both stdweb and wasm_bindgen are being actively maintained and with not so different usage numbers (both somewhat low actually), so IMO we shouldn't abandon either yet.

This is ready for merging then?

@newpavlov
Copy link
Member Author

Yes, it's ready to be merged.

Regarding WASM, I think we should understand how future changes will be handled. Say we publish v0.1 and then stdweb becomes compatible with wasm-bidgen. Or new web/nodejs specific targets will be introduced. The most conservative way will be to publish v0.2 and propagate version bump across other rand crates dependent on getrandom, but I think it will be too much churn. Another approach is to state that both WASM features are experimental and not covered by backward compatibility promise. And the last approach is to move both WASM back-ends to separate crates and tell WASM users to use solution from #4. All three approaches have their own pros and cons, so maybe we should open a separate issue for discussing it.

@dhardy
Copy link
Member

dhardy commented Mar 23, 2019

I think if both stdweb and wasm-bindgen features are enabled, then only wasm-bindgen will actually be used? So if the two libs become compatible, I don't think that's a problem for us.

As for our implementation, if we choose to deprecate one, we can simply add a stdweb feature depending on the wasm-bindgen crate and implementation, so no problem there either that I can see.

@dhardy dhardy merged commit 7e93a69 into rust-random:master Mar 23, 2019
@newpavlov newpavlov deleted the edition branch March 23, 2019 16:12
jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Jul 8, 2019
For the same reasons we started doing it for the main app:
coreos#1719

This time, it's `getrand` that broke us.
rust-random/getrandom#15

We should be able to update to 1.35.0 soon, which will unblock this.
jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Jul 9, 2019
For the same reasons we started doing it for the main app:
coreos#1719

This time, it's `getrand` that broke us.
rust-random/getrandom#15

We should be able to update to 1.35.0 soon, which will unblock this.
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Jul 9, 2019
For the same reasons we started doing it for the main app:
#1719

This time, it's `getrand` that broke us.
rust-random/getrandom#15

We should be able to update to 1.35.0 soon, which will unblock this.

Closes: #1865
Approved by: cgwalters
rh-atomic-bot pushed a commit to coreos/rpm-ostree that referenced this pull request Jul 9, 2019
For the same reasons we started doing it for the main app:
#1719

This time, it's `getrand` that broke us.
rust-random/getrandom#15

We should be able to update to 1.35.0 soon, which will unblock this.

Closes: #1865
Approved by: cgwalters
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.

2 participants