Would it be worth adding a test that ran cabal twice and verified that the cached pkg-config output is used the second time?

I think it would be, I am not sure whether we have anything similar already. I'll check.

Not for caching, but there's a test that verifies that a package reinstall causes a warning IIRC.

this looks excellent, thanks!

Can you describe the cache invalidation strategy?

Does the cache get invalidated when PKG_CONFIG_PATH environment variable gets changed?

It would be good to add some tests testing the different invalidation mechanisms.

Can you describe the cache invalidation strategy?

Yes, I am relying on what getPkgConfigDb does currently:

getPkgConfigDb :: Verbosity -> ProgramDb -> Rebuild PkgConfigDb
getPkgConfigDb verbosity progdb = do
  dirs <- liftIO $ getPkgConfigDbDirs verbosity progdb
  -- Just monitor the dirs so we'll notice new .pc files.
  -- Alternatively we could monitor all the .pc files too.
  traverse_ monitorDirectoryStatus dirs
  liftIO $ readPkgConfigDb verbosity progdb

It obtains PKG_CONFIG_PATH and for monitors each directory for changes. As the comment says, it does not monitor each individual files.

Perhaps it could be worth monitoring the result of getPkgConfigDbDirs itself?

It would be good to add some tests testing the different invalidation mechanisms.

I agree, I am planning to write some.

I did some rework and added tests but one of the tests (ExtraProgPath) seems to have picked up a small difference in behaviour.

@mpickering do you think the tests are sufficient? I am checking the search path (for the pkg-config executable) and PKG_CONFIG_PATH. I don't think it is a perfect solution but it seems to be the approach taken elsewhere.

So from a Nix perspective, the thing that should force cache invalidation is that we have a different pkg-config executable in the two cases. I'm not sure how we should detect that - I guess we could include the fully-resolved path to the executable in the cache key?

I did include the resolved pkg-config location in the key for a bit but eventually removed it because

1. the code flow made it a bit awkward. Both readPkgConfigDb and getPkgConfigDbDirs call needProgram but they don't pass back the resulting programdb so the result is lost.
2. It seems like most of the time we limit ourself to look at the programsearch path

The options I had were

1. call needProgram a third time
2. refactor more code in cabal-install-solver
3. rely on program search path

@fgaz do you mean pkgconfig-depends? zlib (the haskell package) does not use pkg-config by default.

Edit: well well, the program path would also be useless:

❯ nix-shell --pure -p pkg-config --command "type pkg-config; pkg-config --version; pkg-config --variable pc_path pkg-config; pkg-config --list-all"
pkg-config is /nix/store/ljrkf0kwgckcy02kdlz8sjdnxb2pwjb4-pkg-config-wrapper-0.29.2/bin/pkg-config
0.29.2
/nix/store/9z3v4n4lxhqwpi032g1j1a5adp4xxq49-pkg-config-0.29.2/lib/pkgconfig:/nix/store/9z3v4n4lxhqwpi032g1j1a5adp4xxq49-pkg-config-0.29.2/share/pkgconfig

❯ nix-shell --pure -p pkg-config -p zlib --command "type pkg-config; pkg-config --version; pkg-config --variable pc_path pkg-config; pkg-config --list-all"
pkg-config is /nix/store/ljrkf0kwgckcy02kdlz8sjdnxb2pwjb4-pkg-config-wrapper-0.29.2/bin/pkg-config
0.29.2
/nix/store/9z3v4n4lxhqwpi032g1j1a5adp4xxq49-pkg-config-0.29.2/lib/pkgconfig:/nix/store/9z3v4n4lxhqwpi032g1j1a5adp4xxq49-pkg-config-0.29.2/share/pkgconfig
zlib zlib - zlib compression library

I don't think there is any cache to detect this. (PKG_CONFIG_PATH is unset in either case). I'd call this a nixpkgs bug, they need to find a way to make PKG_CONFIG_PATH_FOR_TARGET appear in pkg-config --variable pc_path pkg-config.

What's happening in @fgaz case is that while a success is cached, a failure is not; cabal cannot tell anything has changed so it keeps the previous result (and the rest of the build did not change either).

Give the analysis above, I think this PR is complete. Nevertheless I would like to be sure there is a workaround for the situation that @fgaz describes above.

I realised I didn't need any refactor after all! @fgaz could you give it another go?

@jasagredo does 3ae0488 look alright to you? I tried to follow what you did in #9527

Actual output differs from expected:

stderr:
--- /home/runner/work/cabal/cabal/cabal-testsuite/PackageTests/ExtraProgPath/setup.dist/setup.out.normalized	2024-01-24 14:53:40.341551022 +0000
+++ /home/runner/work/cabal/cabal/cabal-testsuite/PackageTests/ExtraProgPath/setup.dist/setup.comp.out.normalized	2024-01-24 14:53:40.341551022 +0000
@@ -1,8 +1,6 @@
 # cabal v2-build
 Warning: cannot determine version of <ROOT>/./pkg-config :
 ""
-Warning: cannot determine version of <ROOT>/./pkg-config :
-""
 Resolving dependencies...
 Error: [Cabal-7107]
 Could not resolve dependencies:
*** Exception: ExitFailure 1

*** unexpected failure for PackageTests/ExtraProgPath/setup.test.hs

🧐

@michaelpj

So from a Nix perspective, the thing that should force cache invalidation is that we have a different pkg-config executable in the two cases

the pkg-config executable is the same in all my shells. What changes is the non-standard variable $PKG_CONFIG_PATH_FOR_TARGET, confirmed by https://github.com/NixOS/nixpkgs/blob/55ca7939944136731b34316a4c6fc2d4a11da122/pkgs/build-support/pkg-config-wrapper/pkg-config-wrapper.sh

I think there's nothing we can do here unfortunately.

@andreabedini I tried again and the result is identical

the pkg-config executable is the same in all my shells. What changes is the non-standard variable $PKG_CONFIG_PATH_FOR_TARGET, confirmed

oh, in my tests the path to the wrapper was changing :-/

the pkg-config executable is the same in all my shells. What changes is the non-standard variable $PKG_CONFIG_PATH_FOR_TARGET

Right, and in the nix world this is all detectable because the derivation that produces the pkg-config executable is changing, but it's hard for us to perform as fine-grained a check in cabal. It wouldn't be enough to hash the executable, since we also need to know about stuff it uses transitively...

Some ideas:

• Add an environment variable to turn off the caching, tell Nix folks to set it
• Make the cache per-project rather than global. Then somewhat by coincidence this will work out, since the granularity at which Nix folks tend to have different pkg-config executables is per-project. Also I guess a bit more consistent with cabal's other caches, and easily cleanable with cabal clean It already is per-project
• Try and at least advertise that we are getting cached results in solver output, so people know something is up. Especially if thee is a failure...

EDIT: since it's per-project the damage seems less bad, but it maybe forces Nix users to cabal clean when they update their pkg-config dependencies...

I don't have much time to dedicate to this so I would appreciate if anybody could help pushing this over the line. It's complete but I had issues with the tests.

So I tried to run this on Windows again. Some remarks:

1. There are symlinks here. I would advise against that, they do not work on windows, one has to do weird magic tricks to support them.
2. A test is actually failing: PackageTests\CCompilerOverride\setup.test.hs. I will look into it eventually. It says it cannot find the C compiler. It failed because I didn't have clang in my UCRT environment.
3. My ./validate.sh process dies abruptly with this same error I reported time ago: Running the test-suite terminates abruptly with fd:5: hGetLine: end of file in MSYS2 Windows #9571

I got this error on my machine:

fatal: 'C:\msys64\tmp\repos-67360\src' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
Unit Tests
  UnitTests.Distribution.Client.Get
    forkPackages, network tests
      git clone:                                                                                                                       FAIL (2.06s)
        tests\UnitTests\Distribution\Client\Get.hs:219:
        expected a file to exist: C:\msys64\tmp\repos-67360\zlib1/zlib.cabal
        Use -p '/git clone/' to rerun this test only.

1 out of 543 tests failed (77.03s)
<<< C:\Users\Javier\code\cabal\dist-newstyle-validate-ghc-9.8.2\build\x86_64-windows\ghc-9.8.2\cabal-install-3.11.0.0\t\unit-tests\build\unit-tests\unit-tests.exe -j1 --hide-successes (87/104 sec, 1)
<<< C:\Users\Javier\code\cabal\dist-newstyle-validate-ghc-9.8.2\build\x86_64-windows\ghc-9.8.2\cabal-install-3.11.0.0\t\unit-tests\build\unit-tests\unit-tests.exe -j1 --hide-successes (87/104 sec, 1)

^ This was fixed in #9915. Needs a rebase

All tests pass in my machine if using:

• Rebased on top of update zlib.cabal location #9915
• No symlinks
• CLANG64 environment
• Installed pkgconf from mingw-w64-clang64-x86_64-pkgconf
• Used --io-manager=native in a couple of test-suites

FTR: I'm using this to undo-redo the symlinks on Windows

I did the rebase but I some tests failed locally. Perhaps the code has bit-rotted a bit.

You will have to add a step somewhere to install pkg-config on Windows:

Cannot find pkg-config program. Cabal will continue without solving for