Skip to content

pin comfy-table to 7.1.2 #8244

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 29, 2025
Merged

pin comfy-table to 7.1.2 #8244

merged 1 commit into from
Aug 29, 2025

Conversation

@zachschuermann
Copy link
Contributor

@zachschuermann zachschuermann commented Aug 28, 2025 •
edited
Loading

Which issue does this PR close?

What changes are included in this PR?

pin comfy-table to release prior to 7.2.0's MSRV bump to 1.85 - included a TODO to unpin after arrow bumps to 1.85

(context FWIW: caught in delta_kernel MSRV CI)

Are these changes tested?

validated MSRV with cargo-msrv:

# now passes
cargo msrv --path arrow-cast/ verify --rust-version 1.84 --all-features

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 28, 2025
@mbrobbel mbrobbel merged commit f337933 into apache:main Aug 29, 2025
28 checks passed
@mbrobbel
Copy link
Member

Thanks @zachschuermann

@mbrobbel mbrobbel mentioned this pull request Aug 29, 2025
Merged
@scovich
Copy link
Contributor

scovich commented Aug 29, 2025

Would it make sense to ship a point release for this? The current release is unusable because of contradictory MSRV.
To resolve the contradiction, either the MSRV needs to bump or this fix here needs to ship.

@zachschuermann zachschuermann deleted the pin-comfy-table branch August 29, 2025 16:31
@mbrobbel
Copy link
Member

Would it make sense to ship a point release for this? The current release is unusable because of contradictory MSRV. To resolve the contradiction, either the MSRV needs to bump or this fix here needs to ship.

I don't think it really matters. For downstream users without specific MSRV requirements there is no problem, and users with the same MSRV requirements can just pin the version - like we did here. The benefit of a release is that we prevent the impacted users from having to manually pin and/or to avoid an unintended bump of this dependency.

@scovich
Copy link
Contributor

scovich commented Aug 29, 2025

users with the same MSRV requirements can just pin the version - like we did here. The benefit of a release is that we prevent the impacted users from having to manually pin and/or to avoid an unintended bump of this dependency.

Confirmed that delta-kernel-rs was able to pin the version by manually adding it as a dependency (even tho it's not a direct dependency). Annoying but worked.

@debajyoti-truefoundry debajyoti-truefoundry mentioned this pull request Sep 22, 2025
Merged
@geoffreyclaude
Copy link

@zachschuermann Any reason you pinned to 7.1.2 and not a higher patch version (eg, 7.1.4)? Or even version = "~7.1" to force the minor to 7.1 and leave the patch free?
I'm asking because I need 7.1.4 in datafusion-tracing, which is now conflicting with the pinned 7.1.2 of Arrow.

@scovich
Copy link
Contributor

scovich commented Sep 24, 2025

@zachschuermann Any reason you pinned to 7.1.2 and not a higher patch version (eg, 7.1.4)? Or even version = "~7.1" to force the minor to 7.1 and leave the patch free? I'm asking because I need 7.1.4 in datafusion-tracing, which is now conflicting with the pinned 7.1.2 of Arrow.

That may have been an oversight -- delta-kernel-rs is using "~7.1" without MSRV problems.

@geoffreyclaude geoffreyclaude mentioned this pull request Sep 24, 2025
Merged
@zachschuermann
Copy link
Contributor Author

yea that was (1) a typo and (2) in hindsight should have just used "~7.1"! (though it does look like arrow is about to bump to 1.85 anyways?) #8429

@alamb
Copy link
Contributor

alamb commented Sep 24, 2025

@zachschuermann Any reason you pinned to 7.1.2 and not a higher patch version (eg, 7.1.4)? Or even version = "~7.1" to force the minor to 7.1 and leave the patch free? I'm asking because I need 7.1.4 in datafusion-tracing, which is now conflicting with the pinned 7.1.2 of Arrow.

Is this something we should make a patch for 56.2.1 release for?

@alamb
Copy link
Contributor

alamb commented Sep 24, 2025

Here is a PR to unpin comfytable:

@alamb alamb mentioned this pull request Sep 24, 2025
Merged
alamb added a commit that referenced this pull request Sep 25, 2025
@alamb @dependabot
Unpin comfytable (#8440)
f739284 
# Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax.

- Closes #8318
- follow on to #8244

# Rationale for this change

Now that we have updated the MSRV we can unpin the comfy table version
- #8429

# What changes are included in this PR?


# Are these changes tested?



If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?

# Are there any user-facing changes?

If there are user-facing changes then we may require documentation to be
updated before approving the PR.

If there are any breaking changes to public APIs, please call them out.

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbrobbel mbrobbel mbrobbel approved these changes

Assignees

No one assigned

Labels

arrow Changes to the arrow crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

comfy-table release 7.2.0 breaks MSRV

5 participants

@zachschuermann @mbrobbel @scovich @geoffreyclaude @alamb