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

clarify "configure" messages from Cabal #9476

Merged
merged 6 commits into from
Aug 3, 2024
Merged

clarify "configure" messages from Cabal #9476

merged 6 commits into from
Aug 3, 2024

Conversation

geekosaur
Copy link
Collaborator

@geekosaur geekosaur commented Nov 24, 2023

Template Α: This PR modifies cabal behaviour

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!)

@geekosaur
Copy link
Collaborator Author

I used template A because error messages are user-facing. I'm not sure how to test these though, as the only way I know to trigger them is a possible cabal-install bug suffered by @Kleidukos around a week ago.

@Kleidukos
Copy link
Member

I have this bug when using cabal-head but not a released version. Thanks for handling this @geekosaur

@geekosaur
Copy link
Collaborator Author

Sadly I haven't fixed the actual bug, just made the error message a little less confusing. I assume there's a mismatch between what Distribution/Simple/Configure.hs serializes to setup-config and what it expects when it reads it back in later.

@geekosaur
Copy link
Collaborator Author

BTW @Kleidukos can you file a bug about your error so we have a record of it? I may or may not be able to get to it this weekend and I'll be gone on Monday. AFAICT you only mentioned it on Matrix.

@andreabedini
Copy link
Collaborator

LGTM, is Setup clear enough in context?

@geekosaur
Copy link
Collaborator Author

It's used in a few other places (mostly comments), and it could be Setup.lhs instead of Setup.hs.

@geekosaur
Copy link
Collaborator Author

I still don't see a bug corresponding to this, and have no idea whether the original problem still exists, nor how I would go about testing this.

@ulysses4ever
Copy link
Collaborator

@geekosaur do you have a plan for this? SHould it be turned into non-draft?

@Kleidukos
Copy link
Member

I completely missed this. I'll see if it's still a thing.

@geekosaur geekosaur marked this pull request as ready for review June 14, 2024 15:35
@geekosaur
Copy link
Collaborator Author

As far as I'm concerned it's ready to go. But all it does is change some messaging, since Cabal confusingly tells you that you need to reconfigure (which is strictly correct, but if you're using cabal-install then "reconfigure" means something else entirely).

changelog.d/configure-messages Outdated Show resolved Hide resolved
changelog.d/configure-messages Outdated Show resolved Hide resolved
@Kleidukos Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Aug 1, 2024
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 3, 2024
@mergify mergify bot merged commit f203a2c into master Aug 3, 2024
51 checks passed
@mergify mergify bot deleted the configure-messages branch August 3, 2024 19:42
mpickering pushed a commit to mpickering/cabal that referenced this pull request Aug 12, 2024
* clarify "configure" messages from Cabal

* add changelog

* Apply suggestions from code review

Co-authored-by: Javier Sagredo <[email protected]>

---------

Co-authored-by: Javier Sagredo <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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 squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants