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

fix: Use chrono >= 0.4.34, < 0.4.40 to avoid breaking #7210

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Feb 27, 2025

Which issue does this PR close?

Part of #7209

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 27, 2025
@Xuanwo Xuanwo changed the base branch from main to 54.2.0_maintenance February 27, 2025 09:01
@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 27, 2025

cc @alamb and @tustvold for a look, thank you.

@Xuanwo Xuanwo changed the title fix: Use chrono's quarter() to avoid conflict (#7198) fix: Use chrono's quarter() to avoid conflict (cherry-pick) Feb 27, 2025
@tustvold
Copy link
Contributor

tustvold commented Feb 27, 2025

If people may have pinned an older version of chrono, will cargo pick this patch release of arrow and error, or is it smart enough to sort things out?

Wonder if we should use the disambiguification syntax for the backport instead...

Thank you for doing this, I've already spent more time on this chrono nonsense than I hoped to, and have a depressingly long backlog of other work...

Edit: another option would be to just constrain chrono to be less than the borked version... This would likely be the safest option...

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 27, 2025

If people may have pinned an older version of chrono, will cargo pick this patch release of arrow and error, or is it smart enough to sort things out?

cargo will refuse to build if the user tries to use an old version of chrono since we enforce the user to use >= 0.4.40.

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 27, 2025

Thank you for doing this, I've already spent more time on this chrono nonsense than I hoped to, and have a depressingly long backlog of other work...

❤️, happy to provide help.

I noticed your discussion with chrono, and in my opinion, the best approach is to use an API like arrow_xxx to prevent this from happening again. Maybe I should start a dicussion for this?

@tustvold
Copy link
Contributor

tustvold commented Feb 27, 2025

cargo will refuse to build if the user tries to use an old version of chrono since we enforce the user to use >= 0.4.40.

In which case I think we should either change the constrain in this PR to be < 0.4.40 or use the disambiguification syntax. Otherwise people who have fixed their builds will have them break again.

Maybe I should start a dicussion for this?

Having a discussion is probably good just to surface this, although ultimately if chrono takes the policy it is, we effectively need to use disambiguification syntax for all traits where chrono traits are in scope - as they theoretically could conflict with any other trait in context. In this case the collision was with an extension trait, but theoretically the collisions are possible with any trait defined for both a chrono trait and another trait...

It may be the only pragmatic approach is hope they don't break stuff, what the chrono maintainer effectively suggested, which leaves a bad taste in my mouth but may be the only option.

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 27, 2025

In which case I think we should either change the constrain in this PR to be < 0.4.40 or use the disambiguification syntax. Otherwise people who have fixed their builds will have them break again.

Oh, I see. You mean we enforce users to use <0.4.40 in 54.2.1 and >=0.4.40 starting from 55.3.0 (the next minor release). The good thing is that users who have already fixed this issue by pinning chrono won't be affected again. That makes sense. Will do.

@Xuanwo Xuanwo changed the title fix: Use chrono's quarter() to avoid conflict (cherry-pick) fix: Use chrono < 0.4.40 to avoid breaking Feb 27, 2025
@github-actions github-actions bot removed the arrow Changes to the arrow crate label Feb 27, 2025
@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 27, 2025

cc @tustvold, please let me know if this what you want.

Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title fix: Use chrono < 0.4.40 to avoid breaking fix: Use chrono >= 0.4.34, < 0.4.40 to avoid breaking Feb 27, 2025
@tustvold tustvold merged commit ed0dcb5 into apache:54.2.0_maintenance Feb 27, 2025
11 checks passed
jacobtrombetta added a commit to spaceandtimelabs/sxt-proof-of-sql that referenced this pull request Feb 27, 2025
# Rationale for this change
chrono `v0.4.40` implemented a `quarter` method that causes build errors
when building with arrow. See bug
apache/arrow-rs#7196 and this commit
apache/arrow-rs#7210. The suggested workaround
is to pin chrono to `v0.4.39`.

# What changes are included in this PR?
- Pins chrono to the latest version that works with arrow, `v0.4.39`

# Are these changes tested?
Yes
@Xuanwo Xuanwo deleted the backport-chrono branch February 28, 2025 01:36
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