CompatHelper.jl icon indicating copy to clipboard operation
CompatHelper.jl copied to clipboard

Consider the extras block when checking for new versions

Open andreasnoack opened this issue 1 year ago • 11 comments

With the work in https://github.com/JuliaRegistries/CompatHelper.jl/pull/458, it seems like it's pretty simple to close #166 as also conjectured in that issue.

andreasnoack avatar Sep 05 '24 07:09 andreasnoack

Should we make this toggleable? And should it be opt-in or opt-out?

DilumAluthge avatar Sep 05 '24 21:09 DilumAluthge

Some thoughts:

  1. The main use case for [extras] is test-only deps. Most packages in the ecosystem currently don't have [compat] entries for their test-only deps, so this would lead to a surge of new PRs across the ecosystem.
  2. AutoMerge requires packages to have [compat] entries for all regular deps. However, AutoMerge, doesn't currently require packages to have [compat] entries for test-only deps[^1]. CompatHelper was originally invented for the purpose of helping users meet the AutoMerge requirements. In some sense, that continues to be a core purpose of CompatHelper, so perhaps it makes sense that for the default behavior to be congruent with what AutoMerge requires (with of course the ability for users to opt-in to additional functionalities).

[^1]: Maybe we should add that requirement, but that's likely a discussion for a different day.

DilumAluthge avatar Sep 05 '24 21:09 DilumAluthge

There is indeed a significant difference between direct package dependencies and test dependencies in that the latter can't break the environment alone, so having wrong compat (e.g. no) bounds on test dependencies could be considered a private matter. However, I don't really see why it was ever a good idea not to have compat bounds on test dependencies. It's not at all obvious that new versions of test dependencies wouldn't break your tests. However, bottomline here is probably that it should be opt-in. This will require a little more work on the PR, though.

andreasnoack avatar Sep 06 '24 08:09 andreasnoack

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 100.00%. Comparing base (7ea4330) to head (1aea258). :warning: Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #502   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          412       410    -2     
=========================================
- Hits           412       410    -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Sep 25 '24 02:09 codecov[bot]

@DilumAluthge has anything changed with respect to

AutoMerge, doesn't currently require packages to have [compat] entries for test-only deps

or is it status quo?

andreasnoack avatar May 01 '25 13:05 andreasnoack

Nothing has changed, but we might consider making a breaking release of CompatHelper in which we start enforcing compat bounds for test deps (as the default behavior; we could add a kwarg that allows users to opt-out)?

DilumAluthge avatar May 01 '25 23:05 DilumAluthge

IMO it's important to file PRs to adjust bounds for extra deps by default. I just bumped into this case in StatsModels (https://github.com/JuliaStats/StatsModels.jl/pull/323) with CategoricalArrays 1.0. If CompatHelper doesn't notice developers that a new version of the dependency has been released, the package CI will most likely continue testing an old version forever, while most users will use the new one.

To minimize the number of new PRs, would it be possible to only track new versions when version bounds are specified for a given package?

nalimilan avatar Aug 01 '25 07:08 nalimilan

I agree, I also just ran into this with CategoricalArrays 1.0 - tests were still running on an older version and CompatHelper didn't notify me about the release.

@DilumAluthge what's the blocker for the PR? That it would be a breaking change? Maybe it could be softened by a) only opening CompatHelper PRs for dependencies for which already a version bound exists (as suggested in the last comment) and/or b) making it an opt-in feature initially.

devmotion avatar Aug 13 '25 06:08 devmotion

Maybe it could be softened by a) only opening CompatHelper PRs for dependencies for which already a version bound exists (as suggested in the last comment) and/or b) making it an opt-in feature initially.

I think either of those would be a good idea.

DilumAluthge avatar Aug 13 '25 13:08 DilumAluthge

How about an optional kwarg named open_prs_for_extras::Symbol, with the following allowed values:

  1. :all - will open PRs for all [extras]
  2. :none - won't open any PRs for [extras]
  3. :if_existing_compat - if there's an existing compat entry, we'll open PRs, but if there's no existing compat entry, then we won't open a PR.

The kwarg would be optional, and we'd default to :if_existing_compat.

Also, we'd throw an error if any other value (other than the above three values) was passed as the value of open_prs_for_extras.

DilumAluthge avatar Aug 13 '25 13:08 DilumAluthge

I extended the PR in https://github.com/JuliaRegistries/CompatHelper.jl/pull/528 along these lines.

devmotion avatar Aug 13 '25 18:08 devmotion

Closing in favor of #528.

DilumAluthge avatar Dec 12 '25 03:12 DilumAluthge