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

Add Y-forking import project #10508

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

philderbeast
Copy link
Collaborator

Split from #9933, this adds just the project used for that pull request's test without adding any test or behaviour change in cabal.

@philderbeast
Copy link
Collaborator Author

@Mikolaj and @gbaz, I've added this for the reproduction of #10509. Please let me know if you'd like me to add a test for this project to cabal-testsuite/PackageTests/ConditionalAndImport/cabal.test.hs to record the current behaviour in the cabal.out file.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Looks good. Re adding a test, sounds good too, but I don't quite grasp the question: are you asking whether we'd prefer you to add the test sooner rather than later? Or would the test be transient, because the behaviour would be changed soon? Generally, what are the alternatives you ask for recommendation about?

@philderbeast
Copy link
Collaborator Author

@Mikolaj, I've added a simple test that the project builds. That should do for now.

In future, I hope we can avoid the extra work of importing the same config multiple times and if we decide a project import tree like this, with multiple imports of the same .config file, is not an error then at least provide a warning about it.

@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 4, 2024

Makes sense. Thank you.

@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Nov 4, 2024
@philderbeast
Copy link
Collaborator Author

@Mikolaj this hasn't yet merged.

@philderbeast philderbeast force-pushed the repro/config-from-duplicates branch from 3903526 to 563be04 Compare November 11, 2024 15:08
@mergify mergify bot merged commit 287d347 into haskell:master Nov 11, 2024
46 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Nov 11, 2024

Huh, I guess the push made the trick?

@philderbeast
Copy link
Collaborator Author

I wasn't expecting it to merge soon after rebasing. I was surprised that it wasn't reporting any problems before the rebase of today, being a week old. The test results were different. I have no idea why the merge was stuck where it was.

@geekosaur
Copy link
Collaborator

Because Mergify is currently configured to require any merge+no rebase PR to be up to date, because you would need to rebase it manually anyway.

@philderbeast
Copy link
Collaborator Author

Thanks @geekosaur for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants