Skip to content
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

Remove debug-conflict-sets flag from solver package #9432

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented Nov 11, 2023

Fixes #8937.

The debug-conflict-sets build flag probably hasn't been used for a long time, and it isn't currently tested. This commit removes the flag, converts the ConflictSet type back to a newtype, and removes an unnecessary instance.


Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, etc.)

Include the following checklist in your PR:

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

The ticket was opened in May, plenty of time for people to chime in. Adieu debug-conflict-sets.

@grayjay grayjay force-pushed the remove-debug-conflict-sets branch from e47a668 to 4ff0e97 Compare November 15, 2023 01:43
@grayjay
Copy link
Collaborator Author

grayjay commented Nov 15, 2023

Thanks!

@Mikolaj
Copy link
Member

Mikolaj commented Nov 15, 2023

The CI failure seems to be a transient network problem. @grayjay any particular reason to use no_rebase? It produces complex git histories and is less clear/safe regarding what CI tests just before merging.

@grayjay
Copy link
Collaborator Author

grayjay commented Nov 16, 2023

@Mikolaj Thanks for looking into the CI failure. I'll click rerun. I chose "merge+no rebase" because it seemed like the option that would avoid modifying the PR. I think that keeping the original snapshot of the code would be important if the PR involved extensive manual testing, such as running the Hackage benchmarks, but that doesn't apply to this PR. I'll change it to "merge me". I didn't realize that "merge+no rebase" performed fewer checks. Do you know if there is documentation on the process, or does Cabal customize it?

@grayjay grayjay added merge me Tell Mergify Bot to merge and removed merge+no rebase labels Nov 16, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Nov 17, 2023

Hah, rebase vs merge and which keep code integrity better is a holy war topic. :) With relatively low traffic in the cabal repo and few long-running branches, it's decent in both cases, I hope.

I think "merge+no rebase" performs weaker checks in that it runs CI on the original unmerged branch, then merges the PR and only then re-validates the merged commits --- after the PR has landed. I think. Last time I checked. This has its advantages, because the merge queue gets shorter. Would be great to observe in the wild again or look up in some docs.

We have not customized it and we are in the process of documenting the recommended labels and their uses: #9427. Any feedback is very welcome.

I'm taking the liberty of rebasing to restart the CI again, because it seems to have network problems that we can't easily work around.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 17, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 17, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj Mikolaj force-pushed the remove-debug-conflict-sets branch from 4ff0e97 to cf089b6 Compare November 17, 2023 09:46
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 19, 2023
Fixes haskell#8937.

The debug-conflict-sets build flag probably hasn't been used for a long time,
and it isn't currently tested. This commit removes the flag, converts the
ConflictSet type back to a newtype, and removes an unnecessary instance.
@Mikolaj Mikolaj force-pushed the remove-debug-conflict-sets branch from cf089b6 to 9c15880 Compare November 19, 2023 09:48
@mergify mergify bot merged commit c97092f into haskell:master Nov 19, 2023
44 checks passed
@grayjay grayjay deleted the remove-debug-conflict-sets branch December 2, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: solver merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing debug-conflict-sets flag from solver package
4 participants