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

uuid 0.7 not being used because of features in manifest #1900

Closed
2 tasks done
anna-is-cute opened this issue Oct 30, 2018 · 21 comments
Closed
2 tasks done

uuid 0.7 not being used because of features in manifest #1900

anna-is-cute opened this issue Oct 30, 2018 · 21 comments

Comments

@anna-is-cute
Copy link

anna-is-cute commented Oct 30, 2018

Setup

Versions

  • Rust: rust version 1.30.0 (da5f414c2 2018-10-24)
  • Diesel: 1.3.3
  • Database: Any
  • Operating System: Any

Feature Flags

  • diesel: uuid

Problem Description

As far as I can tell, uuid 0.7 will never be used because it doesn't match the requirements. While 0.7 is indeed >= 0.2 and < 0.8.0, it does not have a use_std feature, which locks uuid to 0.6. The feature was renamed to std in 0.7 and is a default feature.

I forked diesel and changed Cargo.toml to use std, then used a patch section in one of my projects. Updating to my fork removed uuid 0.6 and added uuid 0.7. Reverting back to the official git adds uuid 0.6 back.

Checklist

  • I have already looked over the issue tracker for similar issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
@weiznich
Copy link
Member

Uuid 0.7 get's never used because the last release of diesel has no support for uuid 0.7, only master supports the new version.

@anna-is-cute
Copy link
Author

No, master does not. That's why I made this issue.

master explicitly will not use uuid 0.7, since uuid 0.7 misses the feature that master requests.

@weiznich
Copy link
Member

If uuid changed the name of that feature there is no way for diesel to support both versions. That means we cannot update uuid to a newer version without releasing diesel 2.0 because this would remove support for old versions of uuid.

@anna-is-cute
Copy link
Author

Perhaps you should revert #1861 to explicitly show that you won't support uuid 0.7, then.

For what it's worth, use_std was deprecated in uuid 0.6.

@weiznich weiznich reopened this Oct 30, 2018
@weiznich
Copy link
Member

For what it's worth, use_std was deprecated in uuid 0.6.

This doesn't help us because we also support older versions of uuid.

@terry90
Copy link

terry90 commented Nov 12, 2018

If I understand it correctly, uuid won't be updated unless a major version is coming out ? 😢
Is it better to support older versions instead of newer ? It prevents people to update lot of things

Edit: if you want, you can temporarily use:

[patch.crates-io]
diesel = { git = "https://github.com/terry90/diesel.git" }

@weiznich
Copy link
Member

@terry90 That's unfortunate but there is really no way to solve this without a major version bump (or uuid reverts that change…). So nothing we could do about this, sorry.

@weiznich
Copy link
Member

Looked into this a bit further. It seems like we could workaround that issue with the std / no-std feature by just not depending on that feature. But this won't allow us to support uuid 0.7 because they changed some function that we use. So no way to do this update without a breaking change.

@DoumanAsh
Copy link

If you have unstable crate as part of public API, it is already wrong to consider diesel stable. Being part of feature though, I don't see big problem to update in next patch version with mention of need to upgrade uuid, not a big deal

@fairingrey
Copy link

@terry90 I tried using your patch, but I'm getting a ton of errors now and I'm not sure what happened. I've attached the error output here: error.txt

Perhaps this might be related to rust-lang/rust#50504, but I'm not sure why your patch in particular breaks it when it builds just fine without it included.

In regards to the issue, maybe this could be patched in diesel 1.4.x if it's a breaking change? Kind of agree with @DoumanAsh.

@terry90
Copy link

terry90 commented Nov 13, 2018

@fairingrey Sorry I didn't mentioned, you should cargo update or remove your Cargo.lock

@DoumanAsh I agree too, I think I'm not the only one to use Rocket and Diesel, which seems close to a full web framework like Rails with Activerecord, and this issue unfortunately makes the latest diesel incompatible with rocket. The moving parts in the Rust ecosystem - I think - should not be ignored.

If you don't want to make this a breaking change until a minor or major version, another solution would be to maintain a parallel branch, and adds this to the README.md. While I don't think this is the best option, this is something.

Edit: Great, I just saw you started a PR

@weiznich
Copy link
Member

So I've opened a PR that could fix that. Unfortunately that requires an unstable cargo feature, so that's nothing we could merge directly (The required feature seems to be planned for rust 1.31).

If you have unstable crate as part of public API, it is already wrong to consider diesel stable. Being part of feature though, I don't see big problem to update in next patch version with mention of need to upgrade uuid, not a big deal

I agree too, I think I'm not the only one to use Rocket and Diesel, which seems close to a full web framework like Rails with Activerecord, and this issue unfortunately makes the latest diesel incompatible with rocket. The moving parts in the Rust ecosystem - I think - should not be ignored.

That's unfortunate but but there is really nothing we could do about that (Removing uuid from diesel 1.0 is not possible any more …). The only way would be to release diesel 2.0 as soon as possible, but I do not see that happening (because we would like to include some more things in such an release…). Another "solution" would be to break our stability guarantee to make no breaking change, but I think we all agree that we generally do not want to do this (If we do that now, who draws the line when it's ok to do a breaking change and when it's not ok?).

That said both, rocket and uuid have not reached 1.0 yet. That means if you decided to use them you should except breaking things here. (To be clear: It is absolutely ok for them to do this changes, but don't expect a library that has declared some form of stable feature set to follow them.)

For using them as public dependency of a crate that already has reached 1.0 means that you must accept that there may be breaking changes that you cannot support any more. Also that's ok in my point of view (at least as long as the old version does not contain any security issues, which is not the case here).

There are several workarounds for this issue that neither depends on rocket, nor on diesel or uuid.

  • It is possible to add support for own types to diesel. (See this test for an example). This could be used in combination with the new type pattern to add support for whatever uuid version you want outside of diesel.
  • Basically the same is possible on the rocket side

Therefore I do not see this as critical issue that needs to be solved now.

@DoumanAsh
Copy link

That means if you decided to use them you should except breaking things here

That's actually also argument on upgrading your public dependencies until their reach 1.0 (though only as minor version, not patch as user might need to perform actions to ensure compatibility, i.e. upgrading uuid)

Though it is not critical as you say.

@terry90
Copy link

terry90 commented Nov 13, 2018

@weiznich I see your point and I agree. Thanks for digging through

Edit: As @weiznich said, I guess it should look like that https://github.com/terry90/uuid_diesel_0_7_pg so not a big deal. Feel free to use it in your project if you don't want to write it.
This is not usable with the master branch of diesel until #1916 is merged though, I'm using 1.3.3

weiznich added a commit to weiznich/diesel that referenced this issue Nov 27, 2018
Similar to diesel-rs#1900 libsqlite3-dev 0.10.0 is not selected by cargo because
it misses the `min_sqlite_version_3_7_16`. Similar to the situation
there we need to continue to support the old versions (Not because it's
part of a public api in this case, but because it is only possible to
link one version of a native library).
weiznich added a commit to weiznich/diesel that referenced this issue Nov 27, 2018
Similar to diesel-rs#1900 libsqlite3-sys 0.10.0 is not selected by cargo because
it misses the `min_sqlite_version_3_7_16`. Similar to the situation
there we need to continue to support the old versions (Not because it's
part of a public api in this case, but because it is only possible to
link one version of a native library).
@SergioBenitez
Copy link
Contributor

SergioBenitez commented Nov 27, 2018

That said both, rocket and uuid have not reached 1.0 yet. That means if you decided to use them you should except breaking things here. (To be clear: It is absolutely ok for them to do this changes, but don't expect a library that has declared some form of stable feature set to follow them.)

To be clear, strictly speaking, it is not okay, in Rust, for pre 1.0 libraries to have breaking changes between 0.v.a and 0.v.b releases, where b > a. In other words, all 0.v.x releases must be backwards compatible. Rocket takes this very seriously and, to my knowledge, has never violated this guarantee.

I don't have any particularly good ideas about how to resolve the issue at hand, unfortunately. Perhaps Rocket can widen the accepted range of uuid. Unfortunately this isn't a possibility with Rocket as Rocket's Uuid type derefs into the uuid crate's Uuid, so any breaking changes are inherited.

@terry90
Copy link

terry90 commented Nov 27, 2018

@SergioBenitez I don't see the problem, what is the use of Rocket's Uuid aside FromParam ?
Once you have uuid::Uuid, what forbid you to use it like you want ? Here a simple newtype wrapper fixes this issue.
I don't think it's Rocket's responsibility to fix this btw.

@SergioBenitez
Copy link
Contributor

@terry90 The problem, as I stated, is that the wrapper derefs to the internal type, revealing all of its methods. So, if the internal type changes due to a version bump, Rocket user's code breaks.

@terry90
Copy link

terry90 commented Nov 27, 2018

Oh yes, sorry I misunderstood. Then, would it be acceptable to create an additional wrapper type ?

weiznich added a commit to weiznich/diesel that referenced this issue Dec 11, 2018
Similar to diesel-rs#1900 libsqlite3-sys 0.10.0 is not selected by cargo because
it misses the `min_sqlite_version_3_7_16`. Similar to the situation
there we need to continue to support the old versions (Not because it's
part of a public api in this case, but because it is only possible to
link one version of a native library).
emk added a commit to faradayio/falconeri that referenced this issue Dec 18, 2018
`diesel` 1.0 requires `uuid` 0.6, and it cannot support 0.7 without
breaking a stable API. But we can update everything else.

See the extended discussion here:
diesel-rs/diesel#1900
@julien1619
Copy link

Hi! Is there any plan to upgrade uuid despite the strong arguments given here? I would really benefit from this update.

Currently uuid is activated using a feature flag, so, is it possible to add a new feature flag, like uuid-next that will expose an uuid implemented in Diesel code and internally depending on the new version of the uuid crate? It could avoid breaking compatibility, or I misunderstood something?

@weiznich
Copy link
Member

@julien1619 See #1915 😉

@Taha-Firoz
Copy link

@weiznich I can't seem to get Diesel to work with the Rocket Contrib Uuid on version 0.7.4. I can get my project to compile with uuid 0.7.4 however it doesn't work when I run a query with it, the same thing happens when I use a rocket contrib uuid in a query.
However, when I move back my Uuid crate to 0.6 everything works, I can run queries with either the rocket contrib Uuid or the uuid crate's Uuid.

I don't have a problem with using 0.6 but it's confusing me as to why it isn't working since you've added support for 0.7. I also tried moving my version down to 0.7.0 but I can't run any queries until I'm back on 0.6.

Any help would be appreciated, Thank you.

disconsented pushed a commit to NarrativeApp/diesel that referenced this issue Nov 26, 2020
Similar to diesel-rs#1900 libsqlite3-sys 0.10.0 is not selected by cargo because
it misses the `min_sqlite_version_3_7_16`. Similar to the situation
there we need to continue to support the old versions (Not because it's
part of a public api in this case, but because it is only possible to
link one version of a native library).
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

No branches or pull requests

8 participants