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

Don't error if package index is missing (fix #10504) #10506

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Nov 3, 2024

Cherry picked MercuryTechnologies@ec242b1

Would be great to have a test...


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

@gbaz
Copy link
Collaborator

gbaz commented Nov 4, 2024

As per my comments in #10504 I've come around to seeing this as the correct fix. If the missing package index causes problems down the line we'll error anyway. If not, then this is intended behavior and warning is fine.

The semantics of active-repositories is that it picks which of the repositories cabal pulls from are active and in what order -- it was not designed to choose which repositories cabal connects to entirely.

@ulysses4ever
Copy link
Collaborator Author

The test failure is interesting:

-Error: [Cabal-7160]
-The package list for 'repo.invalid' does not exist. Run 'cabal update' to download it.
+Warning: The package list for 'repo.invalid' does not exist. Run 'cabal update' to download it.
+Error: [Cabal-7100]
+There is no package named 'a-b-s-e-n-t'. 
+You may need to run 'cabal update' to get the latest list of available packages.

Arguably, failing earlier in this case is preferable because the reason we failed is not that something is inherently wrong with the a-b-s-e-n-t package but because our repository wasn't configured correctly in the first place, and no matter what package we'll try to get, we'll fail.

So, in this case, I'd argue that reverting the error mode makes the error message worse.

@ulysses4ever
Copy link
Collaborator Author

For posterity: on today's Cabal meeting people agreed that we want to revert it, if only temporarily, until a better solution is found, in the interest of releasing 3.14 without this overly strict behavior.

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Nov 7, 2024
@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 7, 2024
All of the packages Cabal needs may already be present in the local
package database; we don't know yet!

This is a partial revert of #8944.

See: #8944 (comment)
@Mikolaj Mikolaj force-pushed the active-repos-none-broken-10504 branch from 88c4e3d to d58a75e Compare November 9, 2024 20:15
@mergify mergify bot merged commit 6bfdbfd into master Nov 9, 2024
49 checks passed
@mergify mergify bot deleted the active-repos-none-broken-10504 branch November 9, 2024 21:39
@ulysses4ever
Copy link
Collaborator Author

@mergify backport 3.14

Copy link
Contributor

mergify bot commented Nov 10, 2024

backport 3.14

✅ Backports have been created

mergify bot added a commit that referenced this pull request Nov 10, 2024
Backport #10506: Don't error if package index is missing (fix #10504)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport 3.14 merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants