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 dependencies used by PackageTests to exe:cabal-tests #9397

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Nov 4, 2023

The runner allows the tests to use extra dependencies and the custom Prelude from 'exe:cabal-tests'.
However, if the tests use a dependency, say 'directory', and there are two packages with the same unit id available in the store, the test fails since it doesn't know which one to pick.
By including an extra dependency to directory, we force the test runner to use a specific version directory, fixing the test failure.

POC fix for #8356

Please read Github PR Conventions and then fill in one of these two templates.


Include the following checklist in your PR:

@fendor
Copy link
Collaborator Author

fendor commented Nov 4, 2023

Another fix I have considered is to add all dependencies of lib:cabal-tests and exe:cabal-tests together in Setup.hs, giving the tests access to these specific packages.

However, I believe a better long-term solution could be to generate a .ghc.environment file instead of all this compile-time magic in the custom setup.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@fendor
Copy link
Collaborator Author

fendor commented Nov 4, 2023

The fix is, btw, wrong/incomplete. It doesn't fix other issues with the same origin, e.g. #5743. It could be extended to be fixed in the same way, but that's just adding plasters. I think it would be nicer to generate a .ghc.environment file and make runghc use that.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 4, 2023

@fendor: in that case, is this PR a draft? So that we don't merge it by mistake until you are ready...

@fendor fendor marked this pull request as draft November 4, 2023 21:08
@fendor
Copy link
Collaborator Author

fendor commented Nov 4, 2023

@Mikolaj This kind of depends on the active maintainers. I think this should fix CI for immediate issues. However, I think it would be better to migrate the test suite to use environment files. So, whether you merge this PR depends on whether this bug fix is critical right now.

@ulysses4ever
Copy link
Collaborator

My opinion is that it's an important stop-gap fix that should be merged ASAP.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 4, 2023

I see. Then I'm inclined to merge it ASAP as well, but I've asked in the issue ticket just in case and in the interest of transparency.

@fendor fendor marked this pull request as ready for review November 4, 2023 22:17
@ulysses4ever
Copy link
Collaborator

@Mikolaj that makes sense, thank you!

@Mikolaj
Copy link
Member

Mikolaj commented Nov 6, 2023

Nobody came up with a solution that fixes everything from the grounds up, so @fendor, you are our only hope at this time. :) Please go ahead and set the label.

@fendor fendor added the merge me Tell Mergify Bot to merge label Nov 6, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Nov 8, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

Let me prod mergify...

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 9, 2023

rebase

❌ Base branch update has failed

Mikolaj token is invalid, make sure Mikolaj can still log in on the Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

Huh, I do still can login. Let me insist...

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 9, 2023

rebase

❌ Base branch update has failed

Mikolaj token is invalid, make sure Mikolaj can still log in on the Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

@mergify refresh

Copy link
Contributor

mergify bot commented Nov 9, 2023

refresh

✅ Pull request refreshed

Copy link
Member

Mikolaj commented Nov 9, 2023

@Mergifyio rebase

Copy link
Contributor

mergify bot commented Nov 9, 2023

rebase

❌ Base branch update has failed

Mikolaj token is invalid, make sure Mikolaj can still log in on the Mergify dashboard.

Copy link
Member

Mikolaj commented Nov 9, 2023

@Mergifyio requeue default

Copy link
Contributor

mergify bot commented Nov 9, 2023

requeue default

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

I've tried some commands directly from the dashboard, but they seem to do the same thing as from ticket comments (and show up the same and fail the same).

@Mikolaj
Copy link
Member

Mikolaj commented Nov 9, 2023

@fendor: if you feel like it, please kindly rebase manually and merge manually. It may take us a while to diagnose mergify here. No rush, though.

@ulysses4ever
Copy link
Collaborator

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 9, 2023

rebase

❌ Base branch update has failed

ulysses4ever token is invalid, make sure ulysses4ever can still log in on the Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 10, 2023

Let me take the liberty and rebase vis the "Update branch" [edit: rather, "Rebase branch"] button (but first let me try mergify one last time).

@Mikolaj
Copy link
Member

Mikolaj commented Nov 10, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 10, 2023

rebase

❌ Base branch update has failed

Mikolaj token is invalid, make sure Mikolaj can still log in on the Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 10, 2023

Looks fine now, just waiting in the queue (one more PR ahead).

@Mikolaj
Copy link
Member

Mikolaj commented Nov 10, 2023

@mergify rebase

Copy link
Contributor

mergify bot commented Nov 10, 2023

rebase

❌ Base branch update has failed

Mikolaj token is invalid, make sure Mikolaj can still log in on the Mergify dashboard.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 10, 2023

Again the same story (too long on the queue, I guess?). Let's repeat the cure.

The runner allows the tests to use extra dependencies and the custom Prelude
from 'cabal-testsuite'.
However, if the tests use a dependency, say 'directory', and there are two
packages with the same unit id available in the store, the test fails since
it doesn't know which one to pick.
By including an extra dependency to directory, we force the test runner to
use a specific version directory, fixing the test failure.
@mergify mergify bot merged commit 976f86a into haskell:master Nov 10, 2023
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 me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants