Open PRs for compat entries to weak dependencies#458
Open PRs for compat entries to weak dependencies#458DilumAluthge merged 20 commits intoJuliaRegistries:masterfrom
Conversation
|
@DilumAluthge I'm having a little trouble understanding the test suite. Could you direct me to where I would need to make changes to test that an outdated version in |
|
So, the first step is to create some new "templates", which basically just represent what the The templates live here: https://github.com/JuliaRegistries/CompatHelper.jl/tree/master/test/templates Then, for the integration tests, you add these new "template packages" here: https://github.com/JuliaRegistries/CompatHelper.jl/blob/master/test/integration_tests.jl That handles the integration tests. You'll likely also need to add some unit tests for the specific functions modified in this PR. |
|
@DilumAluthge I think this is now ready for review |
|
@sethaxen Can you resolve the merge conflicts? |
|
@mattBrzezinski or @ericphanson Would you be able to take a first pass at reviewing this sometime? |
|
Let's also see if the integration tests pass. bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
I might have some time this week, but I can't make any promises. I'm also offline for the next two weeks following 😬 . |
|
The recent failures are unrelated to this PR. They come from Aqua v0.8, which now enforces that extras have a compat entry (ironically, the failure is because Aqua itself does not have a compat entry). |
| end | ||
|
|
||
| push!(project_deps, compat_entry) | ||
| get!(dep_section, compat_entry, section) |
There was a problem hiding this comment.
what if a package is in multiple sections? E.g. [deps] and [weakdeps] is a documented compatibility strategy
There was a problem hiding this comment.
Ah, I had thought it would make sense to always note that is in [deps] vs [weakdeps] (or in the future [extras]), but yes, the case covered in that docs page is one where one would want to highlight [weakdeps]. Perhaps then it makes sense to list no section in the PR title when the dependency is only in [deps] and otherwise give a comma-separated list of sections (currently [weakdeps] or [deps], [weakdeps])
There was a problem hiding this comment.
If it’s only the title it seems like not too big of a deal either way, just wanted to make sure semantically it was OK. Maybe a simple choice is to just use deps in the title when present by looping over weak deps then deps (or vice versa if you prefer)
There was a problem hiding this comment.
Ah, that's currently how this works. get!(dep_section, compat_entry, section) ensures that dep_section[compat_entry] is only set once to a section for a given package, and if the package was found in the deps section, it's already set, so weakdeps will be ignored.
There was a problem hiding this comment.
If possible, I'd like the titles for regular deps to remain the same as the old titles, and for the new title format to only be used for weakdeps.
There was a problem hiding this comment.
@DilumAluthge this has already been handled in 3e52390
There was a problem hiding this comment.
This looks good to me. It seems slightly unfortunate to produce even more PRs than CompatHelper already does (i.e. we still don't have #126) but that is kind of an orthogonal point.
It would be nice also for CI to be passing, if that is an easy fix to do here. I believe Bors will refuse to merge otherwise.
|
Bump Project.toml version for a feature release (minor bump)? Then I think we're good to go. |
|
BTW I wish we could do incremental rollouts of github actions, e.g. deploy to 1% of users for a day or two to make sure there's no huge issue. Would be great for wide-impact tooling like CompatHelper. I guess we could do something really silly in the action like install a different version of CompatHelper.jl based on |
Yeah it would be nice to have some way if staggering the rollout. |
|
I guess the main thing is we don't get control over the install, as we have users do it themselves: https://github.com/JuliaRegistries/CompatHelper.jl/blob/master/.github/workflows/CompatHelper.yml. If we had our own action that handled the install, then we could implement more sophisticated strategies in it. For example, we could push a release that hardcodes the date, and scales the probability of using the new version by the number of days since that date, so e.g. after 10 days, 100% of the time it uses the new release. If there was a problem, we could then push a new release that reverts to the old version etc. Then next release we could use the time-based rollout again, etc. |
|
bors merge |
|
I guess bors doesn’t listen to me anymore. Maybe @DilumAluthge can do the honors here |
|
@DilumAluthge is there anything else this PR needs? |
|
Nothing needs to be done for the PR. Looks like Eric and I need to figure out how to migrate this repo from Bors to GitHub Merge Queue. |
|
@sethaxen Can you rebase this on the latest master? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #458 +/- ##
===========================================
- Coverage 100.00% 99.50% -0.50%
===========================================
Files 12 12
Lines 398 405 +7
===========================================
+ Hits 398 403 +5
- Misses 0 2 +2 ☔ View full report in Codecov by Sentry. |
|
@DilumAluthge done! |
Fixes #452