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

Improve "Cannot read .cabal file inside ..." errors #10647

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

9999years
Copy link
Collaborator

@9999years 9999years commented Dec 18, 2024

This error message was very confusing -- it doesn't tell you what .cabal file it's looking for or why it cannot read the .cabal file. (Did it fail to parse the .tar archive itself? Did parsing the .cabal file fail? If so, why?)

This patch improves the error message to include the name of the .cabal file being searched for. Additionally, parse errors and warnings are printed, as are format failures in the tarball itself.

I ran into this error while I was writing tests for Cabal and it confused the hell out of me; this is an expanded version of the changes I made to help debug that failure.

Before:

Error: [Cabal-7046]
Cannot read .cabal file inside my-lib-1.0.tar.gz

After:

Error: In <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz: Errors encountered when parsing cabal file my-lib-1.0/my-lib.cabal:

my-lib-1.0/my-lib.cabal:4:22: error:
unexpected Unknown SPDX license identifier: 'puppy' 

    3 | version:        1.0
    4 | license:        puppy license :)
      |                      ^

my-lib-1.0/my-lib.cabal:5:1: warning:
Unknown field: "puppy"

    4 | license:        puppy license :)
    5 | puppy:          teehee!
      | ^
Error: [Cabal-7046]
Failed to read my-lib-1.0/my-lib.cabal from archive <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz
  • Tests have been added (Ask for help if you don’t know how to write them)
    • Manual QA notes have been added
  • A changelog entry has been added in changelog.d/pr-YOUR_PR_NUMBER
  • Documentation has been updated
  • Haddock comments for new top-level definitions have been added
  • base and third-party library imports use qualified imports or explicit import lists
  • Commit messages are formatted nicely
  • Post-merge: Backports for older Cabal release branches have been created

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Can we have (a) a PR template and (b) a changelog entry, since this is user visible? Otherwise, nice.

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.

Nice!

@9999years
Copy link
Collaborator Author

OK, added a changelog entry and updated the test suite. I think moving .cabal file warnings from info to warn resulted in a bunch of output being added to the cli-suite.

@9999years 9999years force-pushed the improve-tar-errors branch 2 times, most recently from 906397d to 97a94a7 Compare December 18, 2024 23:03
This error message was very confusing -- it doesn't tell you what
`.cabal` file it's looking for or why it cannot read the `.cabal` file.
(Did it fail to parse the `.tar` archive itself? Did parsing the
`.cabal` file fail? If so, why?)

This patch improves the error message to include the name of the
`.cabal` file being searched for. Additionally, parse errors and
warnings are printed, as are format failures in the tarball itself.

I ran into this error while I was writing tests for Cabal and it
confused the hell out of me; this is an expanded version of the changes
I made to help debug that failure.

Before:

```
Error: [Cabal-7046]
Cannot read .cabal file inside my-lib-1.0.tar.gz
```

After:

```
Error: In <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz: Errors encountered when parsing cabal file my-lib-1.0/my-lib.cabal:

my-lib-1.0/my-lib.cabal:4:22: error:
unexpected Unknown SPDX license identifier: 'puppy'

    3 | version:        1.0
    4 | license:        puppy license :)
      |                      ^

my-lib-1.0/my-lib.cabal:5:1: warning:
Unknown field: "puppy"

    4 | license:        puppy license :)
    5 | puppy:          teehee!
      | ^
Error: [Cabal-7046]
Failed to read my-lib-1.0/my-lib.cabal from archive <ROOT>/cabal.dist/repo/my-lib-1.0.tar.gz
```
@9999years 9999years marked this pull request as ready for review December 19, 2024 18:41
@9999years 9999years added the merge me Tell Mergify Bot to merge label Dec 19, 2024
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Dec 19, 2024
Copy link
Collaborator

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

I am afraid I have to request changes until the comment below is taken care of. I will push a branch that implements the changes I describe, to merge into this one.

@jasagredo jasagredo removed the ready and waiting Mergify is waiting out the cooldown period label Dec 19, 2024
@jasagredo
Copy link
Collaborator

I'm sorry it seems like it needs a formatting change. Here is a patch to apply
fmt.patch

Copy link
Collaborator

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

Thanks for merging the PR.

@jasagredo
Copy link
Collaborator

Oof we need to add file-uri to the bootstrap jobs. That is kind of easy on Linux and a nightmare on Windows, so if someone else could take care of that it would be awesome.

@jasagredo
Copy link
Collaborator

Aha I see those errors and they are because of file-uri not supporting uri fragments.

ghci> parseFileURI ExtendedPosix "file:/tmp/bar#shared-cache"
Left "endOfInput"
ghci> parseFileURI ExtendedPosix "file:/tmp/bar"
Right (FileURI {fileAuth = Nothing, filePath = "/tmp/bar"})

So I guess we will have to split the string? takeWhile (/= '#')?

@jasagredo
Copy link
Collaborator

jasagredo commented Dec 20, 2024

Here is a patch I would push for this change.
frag.patch
(I can't push to your branch)

@9999years
Copy link
Collaborator Author

@jasagredo I'm going to undo your changes for now, let this get merged with the hack, and then we can see about making it cleaner in a follow-up PR.

This reverts commit 4917d57.
…dows"

This reverts commit 417a1db, reversing
changes made to 0eabec2.
@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 Dec 20, 2024
Copy link
Contributor

mergify bot commented Dec 22, 2024

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated.

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@geekosaur
Copy link
Collaborator

/cc: @9999years

@geekosaur
Copy link
Collaborator

geekosaur commented Dec 22, 2024

You had the bad luck of being in the queue behind another PR, so it needs to be rebased again. It still has merge delay passed, so it should be requeued as soon as you rebase it instead of requiring another 2 day delay.

@mergify mergify bot added the waiting too long automatically set by Mergify to trigger a warning label Dec 26, 2024
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 ready and waiting Mergify is waiting out the cooldown period waiting too long automatically set by Mergify to trigger a warning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants