Skip to content

Conversation

kevinjqliu
Copy link
Contributor

Which issue does this PR close?

  • Closes #.

What changes are included in this PR?

Context: #1195 (comment)
the docs mentions that the feature is not recommended anymore

I've ran an issue with this flag when upgrading datafusion from 47 -> 48. -Z minimal-versions will pull in very old versions of transitive dependencies

Are these changes tested?

@kevinjqliu kevinjqliu changed the title infra: MSRV check, dont use -Z minimal-versions ci: dont use -Z minimal-versions for MSRV check Sep 6, 2025
@kevinjqliu kevinjqliu requested a review from Xuanwo September 7, 2025 19:57
@kevinjqliu kevinjqliu force-pushed the kevinjqliu/remove-msrv-minimal-versions branch from 06dc77a to 32167f0 Compare September 8, 2025 04:11
@liurenjie1024
Copy link
Contributor

I'm +1 with this change, and I think only direct-minimal-versions would meet our requirements?

@kevinjqliu kevinjqliu force-pushed the kevinjqliu/remove-msrv-minimal-versions branch from 32167f0 to 3ba8ed5 Compare September 8, 2025 15:54
@kevinjqliu kevinjqliu changed the title ci: dont use -Z minimal-versions for MSRV check ci: only use -Z direct-minimal-versions for MSRV check Sep 8, 2025
@kevinjqliu
Copy link
Contributor Author

I found some context on using -Z minimal-versions from the original PR
#849 (comment)

@xxchan @Xuanwo wdyt about this change?

@Fokko
Copy link
Contributor

Fokko commented Sep 8, 2025

I'm by no means an expert on the subject, but I don't see many project havingt his check. I would be in favor of dropping -Z minimal-versions

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree direct-minimal-versions seem to be the more wanted semantics, but I remember I met some problems making it work (do not remember the exact problem), thus adding minimal-versions also.

@xxchan
Copy link
Member

xxchan commented Sep 8, 2025

Oh, IIRC, it's because some transitive dependency uses higher MSRV, so minimal-versions can make the problem simple. It seems CI can pass now, so I'm fine with that.

@kevinjqliu kevinjqliu merged commit f47a896 into apache:main Sep 8, 2025
26 of 27 checks passed
@kevinjqliu kevinjqliu deleted the kevinjqliu/remove-msrv-minimal-versions branch September 8, 2025 16:59
@kevinjqliu
Copy link
Contributor Author

thanks everyone!

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.

4 participants