time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.1k stars 277 forks source link

Relax back `serde` dependency #614

Closed pinkforest closed 1 year ago

pinkforest commented 1 year ago

Hello I think the dependency can be now relaxed

https://github.com/serde-rs/serde/releases https://github.com/serde-rs/serde/pull/2590

The precompiled binaries in the existing serde_derive versions have after long manual effort + process been ~reasonably audited / reproducible reverse-engineered (kind of) to some degrees so it may be perhaps fine to relax back to full range of possible versions - looking for input on this.

EDIT: Sadly cargo does not support relaxing two sets of ranges so would have to relax either 1) all or 2) provide cfg-gating

jhpratt commented 1 year ago

Thank you for this information. I will review it later today (I'm about to sleep). Assuming the information is true, which is trivially checked, I will allow those versions greater than or equal to that one. However, I will still prevent use of the versions with precompiled binaries until such time as they can be fully reproduced.

pinkforest commented 1 year ago

We've reproduced reverse-engineered them - but I can add a SemVer :magic_wand: here if you that would be the preference ?

EDIT: Disclaimer - There is no formal manual proof as such and yes I would not accept it either by just believing someone without results :)

kayabaNerve commented 1 year ago

The binaries have not been reproduced @pinkforest.

pinkforest commented 1 year ago

We spent a lot of time reverse-engineering them - but I understand the preference so I will scope out accordingly.

kayabaNerve commented 1 year ago

I wasn't saying they haven't been understood. I was saying your claim, as written, is factually wrong. Not that this is my issue, but I wouldn't personally object to highlighting how understood they are as justification to move on.

pinkforest commented 1 year ago

Manual reproducibility is different than automatic that is true and I understand people want automatic and I fully agree with that. But regardless I will adjust the range. Thanks

EDIT: Disclaimer - There is no formal manual proof as such and yes I would not accept it either by just believing someone without results :)

pinkforest commented 1 year ago

Problem also is there is no way to define multiple ranges in the SemVer statement, I will have to cfg-gate so proposal coming up shortly.

pinkforest commented 1 year ago

Ok latest commit shows alternative approach - see how do you like this approach -

We can gate optionally at --cfg to allow extended version range but by default it would only allow >= x.y.z after the building from source was allowed again.

Is there a way to gate like here: https://github.com/rustsec/advisory-db/pull/1417 - I don't see a way in Cargo ?

pinkforest commented 1 year ago

PR's up:

Couple of approaches there I've outlined - lmk and I can adjust there :heart:

kayabaNerve commented 1 year ago

From my understanding, even attempts at manual reproduction have not produced an exact match.

pythoneer commented 1 year ago

@pinkforest where is the "result" or the steps that lead to the "manual reproduction effort". Or do i need to take your word for it? Sorry that i am unable to find it myself ... the situation is very unclear and messy right now. Is there a central point that explains the situation, makes clear what steps are the result of it and where we stand right now? Currently its all scattered among various PR comments and issues across multiple projects.

pinkforest commented 1 year ago

@kayabaNerve @pythoneer There is no formal proof - that is entirely correct - and it is also correct that it is not that would pass the "stink test" as what we can call entirely reproducible and this is my error.

It is correct that I should have used different wording so thank you on testing me that and I will correct myself up.

My apologies there is no question of my wording being mistaken here -

This also shows that we need to do this more work in the ecosystem via building safeguards until we have tooling for more formalised proofs - and - this is why we have peer-reviews to check our biases / errors in our processes - especially when we have had to revert to manual errorneous process when we've had to adjust our workflows to new challenges that can lead to poor verification when trying to rush with limited resources scrambled together on our free time.

If there is a silver lining - that is that we have more awareness to bring more tooling in like cackle.

Also please see this article by boats - it would be great to solve this as well - but we have to do what we have: https://without.boats/blog/rust-2019/

EDIT: I've corrected what I wrote re: reproducibility - thanks for checking - if it's any salt to teh wounds I was in a rush :woman_facepalming:

pinkforest commented 1 year ago

@kpcyrd Since you've done more documented attempts at this and otherwise great work on making the reproducibility experience much much better :)

By any chance did you happen to get somewhere formally documenting that the binaries on >= 1.0.172, <= 1.0.185 other than the original comments in that origin issue in serde repo ?

p.s. loved the work here: https://github.com/kpcyrd/i-probably-didnt-backdoor-this

Any chance to bring some formality / more documentation into these binaries ?

Cc/ @shnatsel @alex - ideas ? It would be great to be able to relax the time dependency more if poss here ?

roblabla commented 1 year ago

However, I will still prevent use of the versions with precompiled binaries until such time as they can be fully reproduced.

Can we please not? non-semver version constraints tend to break cargo's resolver in very weird ways, inducing more pain than necessary. Now that serde is fixed, the use of precompiled binaries should slowly go away.

BlackDex commented 1 year ago

This constraint also breaks some tooling like cargo outdated ;)

jhpratt commented 1 year ago

@roblabla See #611. The poll had a very clear result.

roblabla commented 1 year ago

@roblabla See #611. The poll had a very clear result.

The MR that was merged uses a normal semver requirement, so I'm happy with the outcome.