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.

Sign up for GitHub

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

Jump to bottom

Add a warning when an env file is created #9705

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Add a warning when an env file is created #9705

merged 6 commits into from
Apr 16, 2024

Conversation

tomsmeding
Copy link
Collaborator

@tomsmeding tomsmeding commented Feb 12, 2024 •
edited
Loading

Inspired by #6481 (comment)

I get that there are many colourable panels on the bikeshed that is cabal env, and even many real engineering choices and challenges, but it would be very nice if at least the user gets some warning if they cabal install --lib and that creates an environment file. After all, statistically, this is not what they intended, or if it was they probably didn't know the consequences. Had they known the consequences, they would have realised that this is not what they wanted.

This patch only shows a warning if an environment file does not yet exist, to acknowledge the set of people who are actually using this functionality.

This is blocked on the bug mentioned here: #9697 (comment)

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

Mikolaj
Mikolaj approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you so much.

cabal-install/src/Distribution/Client/CmdInstall.hs Outdated
++ envFile
++ "\n"
++ "The presence of such an environment file has several knock-on effects. Because\n"
++ "changes the default package set in ghc and ghci from their default (which is\n"
Copy link
Member

Choose a reason for hiding this comment

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

"It changes"? To help distracted readers, I'd twist this sentence around and start with "tools confused" and then explain why confused. I'm wondering how to improve this further. Is the confusion one of the knock-on effects? Or is it a consequence of the effects?

cabal-install/src/Distribution/Client/CmdInstall.hs Outdated
++ "\n"
++ "Double-check that creating a global GHC environment file is really what you\n"
++ "wanted! You can also limit the effects by creating a GHC environment file in a\n"
++ "specific directory only using the --package-env flag, for example:\n"
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid, sometimes it parses with "(only using)" instead of "(directory only)".

@fgaz
Copy link
Member

fgaz commented Feb 12, 2024

I'd also omit the warning if --package-env is used

@tomsmeding
Copy link
Collaborator Author

Thanks @Mikolaj and @fgaz! Very good suggestions, my PR was a bit premature perhaps.

I rebased the branch on the latest master, improved the warning message text, and added a check to only show the message if --package-env is not passed.

There is still this issue I found when testing this, but it feels unrelated.

How do you suggest I test this? A test should be self-contained so it cannot try to put a GHC environment file in the global environment, but making it local with --package-env disables the warning.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 19, 2024

https://ghc.gitlab.haskell.org/ghc/doc/users_guide/packages.html#package-environments suggests you can put the env file in the current directory (e.g., the directory created for the test) and also there's GHC_ENVIRONMENT.

@tomsmeding
Copy link
Collaborator Author

@Mikolaj The point is that I'd like to test that cabal gives the warning at the appropriate time, i.e. when creating a new environment file without --package-env in effect. But in that situation, the environment file gets created globally (in ~/.ghc), which a test shouldn't do! Both because running tests shouldn't interfere with the machine it's running on, and because the tests shouldn't be impacted by whatever the contents of ~/.ghc happen to be on the testing machine. So I'm at a loss for how to test this PR :)

@tomsmeding tomsmeding marked this pull request as ready for review February 19, 2024 10:53
@Mikolaj
Copy link
Member

Mikolaj commented Feb 19, 2024

I see: you are interested in writing the file, not reading it. And it seems GHC_ENVIRONMENT doesn't override ~/.ghc, but adds to it, so it probably doesn't affect where things are written by cabal (I'm don't know).

I wonder how disastrous setting XDG_DATA_HOME to something fake would be...

@fgaz
Copy link
Member

fgaz commented Feb 19, 2024

Tbh I'd probably skip the test. It doesn't seem worth it

@tomsmeding
Copy link
Collaborator Author

I think the PR is ready, albeit without test. Good suggestions for a testing approach are welcome; merging as-is is also fine.

@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024

rebase

✅ Branch has been successfully rebased

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 4, 2024

rebase

✅ Branch has been successfully rebased

@tomsmeding
Copy link
Collaborator Author

I have tested this PR again when rebased to current master and it still works, so I guess it is ready for merging! :)

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label Apr 13, 2024
@Mikolaj
Copy link
Member

Mikolaj commented Apr 13, 2024

Great, it's going to merge in 2 days: https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions.

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

Mikolaj commented Apr 16, 2024

@mergify unqueue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2024

unqueue

☑️ The pull request is not queued

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024

@mergify queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2024

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue 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.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024 •
edited
Loading

Somehow the branch is not modifiable by mergify. I'm going to merge manually, since the delay has passed.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024

@mergify rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2024

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024

@mergify rebase

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2024

rebase

✅ Nothing to do for rebase action

@mergify mergify bot merged commit 00ce024 into haskell:master Apr 16, 2024
49 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024

Huh, mergify merged automatically in the end after the CI finished after a mergify rebase. shrug

@Mikolaj
Copy link
Member

Mikolaj commented Apr 16, 2024

@mergify backport 3.12

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 16, 2024 •
edited
Loading

backport 3.12

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 16, 2024
@tomsmeding @mergify
Add a warning when an env file is created (#9705)
d0060e6 
* Add a warning when an env file is created

#6481 (comment)

* Formatting

* Improve wording of warning message

* Only show warning if --package-env not given

* Improve message and its formatting

* Formatting

(cherry picked from commit 00ce024)
@mergify mergify bot mentioned this pull request Apr 16, 2024
Merged
2 tasks
@tomsmeding tomsmeding deleted the env-file-warning branch April 16, 2024 19:00
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
@tomsmeding @erikd
Add a warning when an env file is created (haskell#9705)
0051a07 
* Add a warning when an env file is created

haskell#6481 (comment)

* Formatting

* Improve wording of warning message

* Only show warning if --package-env not given

* Improve message and its formatting

* Formatting
Mikolaj pushed a commit that referenced this pull request May 1, 2024
@tomsmeding @Mikolaj
Add a warning when an env file is created (#9705)
5b89d82 
* Add a warning when an env file is created

#6481 (comment)

* Formatting

* Improve wording of warning message

* Only show warning if --package-env not given

* Improve message and its formatting

* Formatting

(cherry picked from commit 00ce024)
mergify bot added a commit that referenced this pull request May 1, 2024
@mergify
Merge pull request #9895 from haskell/mergify/bp/3.12/pr-9705
31eb97a 
Add a warning when an env file is created (backport #9705)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geekosaur geekosaur geekosaur approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

Assignees
No one assigned
Labels
cabal-install: environment-files merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days re: install --lib Concerning `cabal install --lib` squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomsmeding @fgaz @Mikolaj @ulysses4ever @geekosaur