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

Make fraction and num-bigint an optional dep of datagen #4458

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Dec 13, 2023

cc @younies : Any new dependencies of the non-experimental ICU4X build should probably be discussed. It's fine as long as this component is experimental, but at some point we need to review if this dependency is the best way to achieve this task.

I'd also like to uplift this to 1.4 and release 1.4.1 , thoughts?

@Manishearth Manishearth added the needs-approval One or more stakeholders need to approve proposal label Dec 13, 2023
@robertbastian
Copy link
Member

At 1.4.0, icu_unitsconversion is a dev-dependency. So if you backport it should be that, not an optional dependency.

@Manishearth
Copy link
Member Author

Good to know, thanks!

I think this dependency isn't used for much (f689152#diff-8e479132faa50170b42b40a4093a8c9f397e4b866e1a03d35d2c5e0e2c336514) and I may make a separate PR removing it entirely.

But for 1.4 we should just devdep it.

@Manishearth
Copy link
Member Author

n.b. we are not in a rush to do this

sffc
sffc previously approved these changes Dec 13, 2023
@Manishearth
Copy link
Member Author

Manishearth commented Dec 13, 2023

I will still merge this regardless since this is not the backport PR. @robertbastian or I will make a backport PR separately.

Though I hope to get rid of this dep entirely

@Manishearth
Copy link
Member Author

autosubmit

@robertbastian
Copy link
Member

num_bigint should get the same treatment

@Manishearth
Copy link
Member Author

Updated to optionalify num-bigint as well.

I'm a bit more okay about the num family of crates since we already depend on some of them, but yeah we shouldn't be adding these deps without discussion anyway.

@Manishearth Manishearth changed the title Make fraction an optional dep of datagen Make fraction and num-bigint an optional dep of datagen Dec 14, 2023
@Manishearth Manishearth merged commit 39139b7 into unicode-org:main Dec 14, 2023
29 checks passed
@Manishearth Manishearth deleted the fractional-dep branch December 14, 2023 00:52
@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Dec 14, 2023
@sffc
Copy link
Member

sffc commented Dec 14, 2023

According to @Manishearth the fraction crate contains substantial unsafe code and therefore is a nontrivial import. @Manishearth will make a PR that attempts to remove the fraction dependency.

Manishearth added a commit that referenced this pull request Dec 19, 2023
Backports #4459 and
#4458

Backport approved in #4458

Unlike that PR, this makes these deps dev-deps since they're not yet
used by an experimental feature
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.

3 participants