-
Notifications
You must be signed in to change notification settings - Fork 39
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
Open PRs for compat entries to weak dependencies #458
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DilumAluthge this has already been handled in 3e52390
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
18bd2a4
Fixes #452