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

mysql_async, disable default features (#285) #286

Merged
merged 4 commits into from
Sep 10, 2023
Merged

mysql_async, disable default features (#285) #286

merged 4 commits into from
Sep 10, 2023

Conversation

glueball
Copy link
Contributor

Disable default features for mysql_async. Enable only the minimal feature, which is required by the mysql_crate.

Also, removed the dependency with the tokio feature. Doesn't seem to be needed, since tokio isn't mentioned at all in the refinery_core::drivers::mysql_async module.

I was not sure how to test this, I did this and everything works:

test --workspace -F mysql_async -- --test-threads=1

@glueball
Copy link
Contributor Author

Also, I think you could enable "minimal" in regular mysql instead of the manual flate2 dependency.

Some other dependencies could have some features removed also. Most notably tokio, is really "full" needed by refinery? Enabling features in refinery prevents downstream users from disabling them.

@jxs
Copy link
Member

jxs commented Sep 10, 2023

Hi, and thanks for this! ❤️

Also, I think you could enable "minimal" in regular mysql instead of the manual flate2 dependency.

Some other dependencies could have some features removed also. Most notably tokio, is really "full" needed by refinery? >Enabling features in refinery prevents downstream users from disabling them.

Yeah, with time I have been made aware (and suffered because 😅 ) of that, do you wanna go ahead and do it on this PR? we can update the wording to reflect the changes, I can then release a tiny as this shouldn't be breaking.

@glueball
Copy link
Contributor Author

Yeah I can add that to the pr!

@glueball
Copy link
Contributor Author

However, maybe this can be considered breaking. Not sure what the actual policy for that is.

Some downstream projects might be using features enabled now by refinery that are not listed in their own Cargo.toml. The fix is trivial, and if using rust 1.72 even the compiler warns what features to enable; but technically can be considered breaking.

@glueball
Copy link
Contributor Author

glueball commented Sep 10, 2023

Used cargo hack to check all possible feature combinations

cargo hack check --feature-powerset --no-dev-deps --skip default --workspace

I also took the liberty to update siphasher to 1.0, as my IDE was giving me a warning. The hasher seems to be covered by migration divergence tests, so if CI turns green it should be safe.

refinery_core/Cargo.toml Outdated Show resolved Hide resolved
refinery_core/Cargo.toml Outdated Show resolved Hide resolved
refinery_core/Cargo.toml Outdated Show resolved Hide resolved
@jxs
Copy link
Member

jxs commented Sep 10, 2023

However, maybe this can be considered breaking. Not sure what the actual policy for that is.

Some downstream projects might be using features enabled now by refinery that are not listed in their own Cargo.toml. The fix is trivial, and if using rust 1.72 even the compiler warns what features to enable; but technically can be considered breaking.

yeah thanks for mentioning, I can release a minor instead

I am not at home currently, will try to do that Tomorrow!

@jxs jxs merged commit 4326cbc into rust-db:main Sep 10, 2023
27 checks passed
@jxs
Copy link
Member

jxs commented Sep 14, 2023

thinking more about it, I am willing to run that risk. I hope there are not a lot of users doing it, but misusing the feature set should not be considered a feature I'd say, therefore we won't be breaking anything.
released 0.8.11

@glueball
Copy link
Contributor Author

Great, thanks!

As a refinery user, I'm really not concerned about this change as I do usually manage all the features of my dependencies (but the rest of the team always turns to me to fix it when something breaks :P).

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