Skip to content

Conversation

@andreasnoack
Copy link
Collaborator

Test on min, lts, 1, and pre instead of hard coded versions and nightly.

Test on min, lts, 1, and pre instead of hard coded versions and nightly.
@andreasnoack andreasnoack enabled auto-merge (squash) August 3, 2025 15:52
@andreasnoack andreasnoack disabled auto-merge August 3, 2025 15:52
@andreasnoack andreasnoack merged commit b7564b6 into main Aug 3, 2025
22 checks passed
@andreasnoack andreasnoack deleted the andreasnoack-patch-1 branch August 3, 2025 16:08
@LilithHafner
Copy link
Member

LilithHafner commented Aug 3, 2025

We should continue to test this package on nightly. This PR was also merged with unreviewed changes. @andreasnoack, would you please explain those changes.

@andreasnoack
Copy link
Collaborator Author

andreasnoack commented Aug 3, 2025

I'm sorry for moving a bit fast here. A million packages are being hold back by the new DataStructures release. I saw that the most recent release here was quite old and wanted to help with unblocking the upgrades. If you are activately maintaining this package then please feel free to revert whatever your don't think is right. It would be good to get a new release out fairly soon that allows for the new DataStructures release, though.

We should continue to test this package on nightly.

What is the motivation for that? It is very noisy for CI and taxes package maintenance. In most cases for very little benefit.

would you please explain those changes.

It is just an easier way to enforce the same checks as before. With a finalize step, you don't have to list all the individual jobs that are required, and particularly, you avoid having redundantobsolete checks which happened in this case and was the reason I made the change. I've made that change in many packages at this point and I learned it from Dilum so I was certain that he was okay with it which was why I took the liberty to merge without his renewed approval.

@LilithHafner
Copy link
Member

I can understand why you looked at the date and assumed it was unmaintained. I do actively maintain this package. It's a low-churn package so that's why the most recent commit was very old.

The reason why I test on nightly is threefold:

  • This package depends on Julia's sorting internals so it can be expected to break on non-breaking Julia versions. Thus failures on nightly may well be package-author responsibility in this case.
  • I'm the primary maintainer of both this package and Julia's Base sorting algorithms and internals. So even if the failure is Julia's fault, it's still likely my fault and I want to know about it.
  • With 3k dependents, if we want the package ecosystem to be at all functional on nightly (which I do), we should make sure this package is functional on nightly.

It is just an easier way to enforce the same checks as before.

While I don't necessarily disagree, I still don't understand that code and I'd rather just stick with what was working before. I need a little more motivation to add a new github actions syntax construct to CI here.

Thanks for trying to help out here and thanks for working on getting DataStructures.jl 0.19 propagated.

LilithHafner added a commit that referenced this pull request Aug 4, 2025
LilithHafner added a commit that referenced this pull request Aug 4, 2025
* Revert "Adjust CI Julia versions (#102)"

This reverts commit b7564b6.

* Keep the addition of `pre` and `lts`
@andreasnoack
Copy link
Collaborator Author

While I don't necessarily disagree, I still don't understand that code and I'd rather just stick with what was working before.

I encourage you to stick with the new version. It is much easier to work with, so let me try to explain it. It defines a tiny extra job that depends on the test job to finish successfully. This allows you to just specify the finalize job as required before merging. The problem with the previous approach is that you have to select each of the action that are required and it matches on all parts of the build matrix. Hence, each change to the matrix will leave some required actions undefined such that the PR cannot be merged until you have changed the requirements in the branch protection.

@LilithHafner
Copy link
Member

Right now most but not all jobs are required (e.g. nightly is not required). This is trivial with the current approach, as is adjusting which jobs are required, at the discretion of a repo-admin, based on their judgement of the acceptability of a failure. How is that done with your proposed approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants