Fix default Nix configuration option in generated ~/.cabal/config file #8878

cbclemmer · 2023-03-28T14:28:48Z

Please 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.

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

Should close #8864

In PR #8522, I did not test the default generated config, and made an incorrect assumption about its contents. This led to the Nix configuration option being uncommented in the ~/.cabal/config file when generated for a user for the first time.

In this PR, I have updated the default value of the Nix configuration option to be empty. This causes the pretty print function, which writes the default config, to comment out the Nix configuration option. This change ensures compatibility, allowing newer Cabal versions to generate configuration files suitable for older Cabal versions.

ulysses4ever · 2023-03-28T14:36:30Z

Thank you!

chreekat · 2023-04-05T06:16:23Z

Just an observation from a non-maintainer: one thing that would make this patch more valuable than it already is would be to add some documentation to the test, and maybe clarify the comment on the implementation (why do we "need to return PP.empty from viewAsFieldDescr").

gbaz · 2023-04-06T17:11:57Z

cabal-install/tests/IntegrationTests2.hs

+
+  defaultConfigFile <- readFile (basedir </> "nix-config/default-config")
+  True @=? isInfixOf "-- nix:\n" defaultConfigFile
+


can we generalize this test to ensure that essentially all options are commented out in the default config, not just nix?

i.e. the repo perhaps and a few other things should not be commented, but it would be much better to have a much more uniform test.

Mikolaj · 2023-04-19T20:58:03Z

Could we move this forward? The world needs this PR.

Mikolaj · 2023-05-04T18:06:24Z

@mergify rebase

mergify · 2023-05-04T18:06:31Z

rebase

✅ Branch has been successfully rebased

Mikolaj · 2023-05-04T18:06:46Z

Let me prod the CI...

Mikolaj · 2023-05-04T18:37:46Z

Failed again. Rerunning failed jobs...

Mikolaj · 2023-05-04T20:26:16Z

@cbclemmer: it seems, the new test started failing on Windows after updating to new GHC or after some changes of setup of the CI machines. Are you able to guess what's going on with this access denied problem? The path is looking funny, but I'm not sure what I'm looking at.

cbclemmer · 2023-05-05T14:39:12Z

@Mikolaj I added a test that will update cabal-install/tests/IntegrationTests2/config/default-config with whatever the current default is and I'm not sure why it's failing. It must have something to do with the CI file permissions because the test works on my machine

Mikolaj · 2023-05-05T15:46:52Z

Uhoh. Let's try to debug. Does anybody have any ideas? The error is

Exception: tests\IntegrationTests2\config/default-config.tmp: renameFile:renamePath:MoveFileEx "\\\\?\\D:\\a\\cabal\\cabal\\cabal-install\\tests\\IntegrationTests2\\config\\default-config.tmp" Just "\\\\?\\D:\\a\\cabal\\cabal\\cabal-install\\tests\\IntegrationTests2\\config\\default-config": permission denied (Access is denied.)

Printing both paths aligned:

Exception: tests\IntegrationTests2\config/default-config.tmp: renameFile:renamePath:MoveFileEx 
"\\\\?\\D:\\a\\cabal\\cabal\\cabal-install\\tests\\IntegrationTests2\\config\\default-config.tmp" Just 
"\\\\?\\D:\\a\\cabal\\cabal\\cabal-install\\tests\\IntegrationTests2\\config\\default-config": permission denied (Access is denied.)

shows they differ just by the .tmp. Do you recognize the paths? Should the \\config\\ directory exist by that point?

cbclemmer · 2023-05-05T15:56:00Z

@Mikolaj The file should be committed and present at that time, but I've also tried it with the file and it gives the same error. The test that fails writes the file to the folder, so it would override it. Now that I think of it, the nix flag test does the same thing in the test above it so it may not have closed the file handle properly. I've added a commit that will remove the reference to the default config in the nix test, that may help

Mikolaj · 2023-05-05T18:45:02Z

I guess that's progress?

Test Default Flags:             FAIL
      tests\IntegrationTests2.hs:1975:
      expected: True
       but got: False

Mikolaj · 2023-05-11T17:35:27Z

@cbclemmer: Joking aside. is this test result now explainable?

cbclemmer · 2023-05-15T16:27:14Z

@Mikolaj Sorry, I've been super busy lately but recently I have had more time. The issue was that Windows used a different setting for that parameter than Linux. To fix this I first changed the test to compare the strings on the assertion rather than an auxiliary function, which let me see what the matching line said and allowed me to come to the explanation above. Then to fix it I just compared the string before the colon so that the system differences for the values (and the ones that included the user's home directory) would not be tested because that's not the point of this test anyway. Then I went through all of the default config values and ensured that they are all present and tested for whether they are commented or not. The only drawback to my approach is that it does not test whether specific options are under a heading, but it does test whether they are indented or not. Everything should be good to go now.

Mikolaj · 2023-05-15T19:12:51Z

Good sleuthing. Would you set the squash+merge_me label?

cbclemmer · 2023-05-16T13:54:16Z

@Mikolaj Done

Mikolaj · 2023-05-18T16:56:14Z

Thank you again!

Mikolaj · 2023-05-18T18:13:53Z

@mergify backport 3.10

mergify · 2023-05-18T18:13:56Z

backport 3.10

✅ Backports have been created

#8955 Fix default Nix configuration option in generated ~/.cabal/config file (backport #8878) has been created for branch 3.10

#8878) * default to commented line for nix config * Fix whitespace * Remove vscode folder * Remove default config file for test * Remove test generated file * Add changelog * Add test for default config values * Remove config file * Remove parsing file * Add config file back for tests * Remove reference to default config to try to get the new test to pass * Rewrite test to be more verbose WIP * Stop testing for values and only test for whether it is commented or not * Fill out rest of tests WIP * Clean up --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 9bf415a)

Fix default Nix configuration option in generated ~/.cabal/config file (backport #8878)

haskell#8878) * default to commented line for nix config * Fix whitespace * Remove vscode folder * Remove default config file for test * Remove test generated file * Add changelog * Add test for default config values * Remove config file * Remove parsing file * Add config file back for tests * Remove reference to default config to try to get the new test to pass * Rewrite test to be more verbose WIP * Stop testing for values and only test for whether it is commented or not * Fill out rest of tests WIP * Clean up --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

cbclemmer mentioned this pull request Mar 28, 2023

Fresh ~/.cabal/config written by cabal 3.10 crashes any older cabal #8864

Closed

ulysses4ever approved these changes Mar 28, 2023

View reviewed changes

Mikolaj added the attention: needs-review label Mar 28, 2023

gbaz reviewed Apr 6, 2023

View reviewed changes

gbaz approved these changes Apr 20, 2023

View reviewed changes

cbclemmer added 10 commits May 4, 2023 18:06

default to commented line for nix config

8945a7a

Fix whitespace

1eaf42a

Remove vscode folder

6723843

Remove default config file for test

cc10327

Remove test generated file

65240be

Add changelog

a9e8949

Add test for default config values

e69e141

Remove config file

a2cdd48

Remove parsing file

65ab79f

Add config file back for tests

e40d0ef

Mikolaj force-pushed the colton/#8864 branch from 64d22f5 to e40d0ef Compare May 4, 2023 18:06

Remove reference to default config to try to get the new test to pass

a28fa65

cbclemmer added 4 commits May 12, 2023 18:01

Rewrite test to be more verbose WIP

e6db92d

Stop testing for values and only test for whether it is commented or not

cd07310

Fill out rest of tests WIP

dc5d15d

Clean up

f152568

cbclemmer added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels May 16, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 18, 2023

Merge branch 'master' into colton/haskell#8864

62610f9

mergify bot merged commit 9bf415a into haskell:master May 18, 2023

mergify bot mentioned this pull request May 18, 2023

Fix default Nix configuration option in generated ~/.cabal/config file (backport #8878) #8955

Merged

mergify bot added a commit that referenced this pull request May 19, 2023

Merge pull request #8955 from haskell/mergify/bp/3.10/pr-8878

5a83705

Fix default Nix configuration option in generated ~/.cabal/config file (backport #8878)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix default Nix configuration option in generated ~/.cabal/config file #8878

Fix default Nix configuration option in generated ~/.cabal/config file #8878

cbclemmer commented Mar 28, 2023 •

edited

Loading

ulysses4ever commented Mar 28, 2023

chreekat commented Apr 5, 2023

gbaz Apr 6, 2023

gbaz Apr 6, 2023

Mikolaj commented Apr 19, 2023

Mikolaj commented May 4, 2023

mergify bot commented May 4, 2023

Mikolaj commented May 4, 2023

Mikolaj commented May 4, 2023

Mikolaj commented May 4, 2023

cbclemmer commented May 5, 2023

Mikolaj commented May 5, 2023 •

edited

Loading

cbclemmer commented May 5, 2023

Mikolaj commented May 5, 2023

Mikolaj commented May 11, 2023

cbclemmer commented May 15, 2023

Mikolaj commented May 15, 2023

cbclemmer commented May 16, 2023

Mikolaj commented May 18, 2023

Mikolaj commented May 18, 2023

mergify bot commented May 18, 2023 •

edited

Loading


		defaultConfigFile <- readFile (basedir </> "nix-config/default-config")
		True @=? isInfixOf "-- nix:\n" defaultConfigFile

Fix default Nix configuration option in generated ~/.cabal/config file #8878

Fix default Nix configuration option in generated ~/.cabal/config file #8878

Conversation

cbclemmer commented Mar 28, 2023 • edited Loading

ulysses4ever commented Mar 28, 2023

chreekat commented Apr 5, 2023

gbaz Apr 6, 2023

Choose a reason for hiding this comment

gbaz Apr 6, 2023

Choose a reason for hiding this comment

Mikolaj commented Apr 19, 2023

Mikolaj commented May 4, 2023

mergify bot commented May 4, 2023

✅ Branch has been successfully rebased

Mikolaj commented May 4, 2023

Mikolaj commented May 4, 2023

Mikolaj commented May 4, 2023

cbclemmer commented May 5, 2023

Mikolaj commented May 5, 2023 • edited Loading

cbclemmer commented May 5, 2023

Mikolaj commented May 5, 2023

Mikolaj commented May 11, 2023

cbclemmer commented May 15, 2023

Mikolaj commented May 15, 2023

cbclemmer commented May 16, 2023

Mikolaj commented May 18, 2023

Mikolaj commented May 18, 2023

mergify bot commented May 18, 2023 • edited Loading

✅ Backports have been created

cbclemmer commented Mar 28, 2023 •

edited

Loading

Mikolaj commented May 5, 2023 •

edited

Loading

mergify bot commented May 18, 2023 •

edited

Loading