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

Ignore ~/.cabal if $XDG_CONFIG_HOME/cabal/config exists. #8877

Merged
merged 6 commits into from
May 24, 2023

Conversation

athas
Copy link
Collaborator

@athas athas commented Mar 28, 2023

This is a draft implementation of #8577.

There is one flaw in this implementation: the warning is printed many times, and with non-configurable verbosity. Changing that would require one of:

  1. Significant refactoring of how getDefaultDir is called, which has non-local implications.
  2. Doing the warning somewhere else (e.g. in Main.hs).

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@athas athas marked this pull request as ready for review March 28, 2023 05:45
@athas
Copy link
Collaborator Author

athas commented Mar 28, 2023

It now warns only when loadConfig is called, but for some commands (e.g. cabal install) that is still twice.

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.

It’s my understanding that there’s no way we can test that it does the right thing, is there?

@ulysses4ever
Copy link
Collaborator

I’m not worried about repetitive warnings: it won’t happen too often, and really the user should make sure that there’s only one thing exists.

@athas
Copy link
Collaborator Author

athas commented Apr 4, 2023

It’s my understanding that there’s no way we can test that it does the right thing, is there?

In principle it is possible to do so in an ad-hoc manner (after all, I tested it manually), but I don't know whether cabal's test suite is set up to make it easy.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 19, 2023

What's the status here? Let me add a needs-review label in case it's ready for review...

@athas
Copy link
Collaborator Author

athas commented Apr 20, 2023

I don't remember anything left to do here. There are no tests, though.

@Kleidukos
Copy link
Member

@athas If you could write down some instructions for QA testers so that they can manually check your patch, that would be grand. :)

@athas
Copy link
Collaborator Author

athas commented May 18, 2023

Sure. There are the following situations to test, based on which files and directories exist. Their contents do not matter.

  1. Neither $HOME/.cabal nor $HOME/.config/cabal/config exist. In this case cabal help should print $HOME/.config/cabal/config on the second-to-last line.
  2. $HOME/.cabal exists as a directory and $HOME/.config/cabal/config does not exist. cabal help should print $HOME/.config/cabal/config on the second-to-last line (or last line if $HOME/.cabal/config actually exists).
  3. $HOME/.cabal exists as a directory but $HOME/.config/cabal/config also exists. cabal help should print $HOME/.config/cabal/config on the last line.
  4. $HOME/.cabal does not exist but $HOME/.config/cabal/config exists. cabal help should print $HOME/.config/cabal/config on the last line.

@Mikolaj
Copy link
Member

Mikolaj commented May 22, 2023

@athas: it's normally PR author's role, but let me set the label in the interest of merging this, before CI breaks or something. :) This will also unblock QA, which happens after the merge.

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label May 22, 2023
@Mikolaj
Copy link
Member

Mikolaj commented May 22, 2023

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented May 22, 2023

backport 3.10

✅ Backports have been created

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 24, 2023
@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@mergify unqueue

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

unqueue

☑️ The pull request is not queued

@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

queue

🛑 The pull request has been removed from the queue

Pull request #8877 has been dequeued due to failing checks or checks timeout.

You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@ulysses4ever
Copy link
Collaborator

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

refresh

✅ Pull request refreshed

@ulysses4ever
Copy link
Collaborator

There's a funny Mergify failure here :

The merge queue pull request can't be updated
Details:

    Base branch update has failed

    refusing to allow a GitHub App to create or update workflow .github/workflows/quick-jobs.yml without workflows permission
    err-code: CDDDA

I don't understand why Mergify (apparently) tries to modify the workflow file...

@ulysses4ever
Copy link
Collaborator

Googling gives this: Mergifyio/mergify#5055 supposedly resolved by this comment:

This is a GitHub App permission problem. If you log into your Mergify dashboard you should see a message requesting you to update the permission for the app.

I don't see anything like that in the dashboard but on one of the tabs it says that I should be an GH Org admin to fiddle with "Application keys", which I'm not sure whether it's related or not. https://dashboard.mergify.com/github/haskell/application-keys

@athas
Copy link
Collaborator Author

athas commented May 24, 2023

Is something more expected of me? It's not that I would mind writing automated tests, but someone gave me the impression that it would not be easy to fit into cabal's current test infrastructure. I'm fine with adding some more furniture, but I don't currently have the time to rework the plumbing.

@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@athas: I wasn't the reviewer, so I don't know, but this PR is certainly ready to be merged and 3.10.2.0 release is blocked by it, so we are currently most of all fighting the random technical obstacles to that effect. Thank you for asking, though. :)

@Mikolaj
Copy link
Member

Mikolaj commented May 24, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented May 24, 2023

rebase

✅ Branch has been successfully rebased

@ulysses4ever
Copy link
Collaborator

@athas I think your job is done here, thanks a lot! Let's keep in touch: XDG is a rather fundamental change, and I expect more questions will need to be discussed in the future.

@mergify mergify bot merged commit 784d13b into haskell:master May 24, 2023
mergify bot pushed a commit that referenced this pull request May 24, 2023
* Ignore ~/.cabal if $XDG_CONFIG_HOME/cabal/config exists.

* Also document this.

* Slightly fewer warnings.

* Use verbosity flag.

* Better text.

* Oops

(cherry picked from commit 784d13b)
mergify bot added a commit that referenced this pull request May 24, 2023
Ignore ~/.cabal if $XDG_CONFIG_HOME/cabal/config exists. (backport #8877)
@jgotoh jgotoh self-assigned this May 27, 2023
@jgotoh
Copy link
Member

jgotoh commented May 27, 2023

Manual QA succeeded for the provided steps, if anyone is interested here is the console output:
https://gist.github.com/jgotoh/1626aba91f742b74629c689a9af827d6

@ulysses4ever
Copy link
Collaborator

Great! 🎉

@Mikolaj
Copy link
Member

Mikolaj commented Aug 31, 2023

Thanks, this works for me in the semi-wild with cabal 3.10.2.0-alpha1. I had no idea I have cabal configs in both locations and it detected it. Here's my battle report from the Matrix channel:


FYI, I'm dog-fooding the centos7 flavour of cabal 3.10.2.0-alpha1 from https://gitlab.haskell.org/haskell/cabal/-/jobs/1651797/artifacts/browse/out/
so far, so good, it builds stuff fine (though there is a slight inconsistency in the message below, as it mentions a directory first and a (config) file in an analogous directory second)

123456789~/r/horde-ad$ cabal build horde-ad-simplified --enable-optimization 
Warning: Both /home/mikolaj/.cabal and /home/mikolaj/.config/cabal/config
exist - ignoring the former.
It is advisable to remove one of them. In that case, we will use the remaining
one by default (unless '$CABAL_DIR' is explicitly set).
Warning: Both /home/mikolaj/.cabal and /home/mikolaj/.config/cabal/config
exist - ignoring the former.
It is advisable to remove one of them. In that case, we will use the remaining
one by default (unless '$CABAL_DIR' is explicitly set).

(and the message is repeated twice, but that's probably unavoidable; IDK)
in any case, it detected my duplicated cabal config and warned me, so it prevents real user problems

@Kleidukos
Copy link
Member

Kleidukos commented Aug 31, 2023

We have the option of mentioning $CABAL_CONFIG instead.

We can also link to the manual: https://cabal.readthedocs.io/en/3.10/config.html#configuration-file-discovery

@Kleidukos
Copy link
Member

@Mikolaj Do you think this warrants a patch?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 31, 2023

If I'm wrong and the message duplication is easy to fix, then a patch would be helpful. Then we could also make the mentions of directory /home/mikolaj/.cabal vs file /home/mikolaj/.config/cabal/config consistent and mention $CABAL_CONFIG instead or in addition to $CABAL_DIR (I'm happily ignorant about what they do).

I think there's absolutely no rush. The problems are minor and the changes will be backward compatible, so they don't need to be included this release. But I may be misunderstanding any of the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-manual-qa PR is destined for manual QA attention: needs-review 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
Status: Testing Approved
Development

Successfully merging this pull request may close these issues.

6 participants