Skip to content

Fix ci cache for windows#2507

Merged
mergify[bot] merged 4 commits into
haskell:masterfrom
jneira:fix-windows-cache
Dec 21, 2021
Merged

Fix ci cache for windows#2507
mergify[bot] merged 4 commits into
haskell:masterfrom
jneira:fix-windows-cache

Conversation

@jneira

@jneira jneira commented Dec 18, 2021

Copy link
Copy Markdown
Member
  • The cache in windows is not working from some time ago. I did not find any job where the build times are below 40 min since several days ago.
  • They are downloading all the packages (so the source cache is not working) and building all the packages (so the cabal store is not working)
  • I have an hypothesis: both caches are failing due to different reasons
    • Source cache is failing cause we were using the default path for CABAL_DIR: \\AppData\\cabal in windows and ~/.cabal in the other os
    • cabal store cache is failing due to a bug in the haskell setup action:
      • we are using wisely steps.HaskEnvSetup.outputs.cabal-store but as you can see in this run the effective cache action is:
Run actions/cache@v2
  with:
    path: C:\sr
    key: compiled-deps-Windows-9.0.1-2021-11-29T12-30-07Z-0c0e51e7952f2bb6f6a68b93790dcd1c9321c830a6ec4745f9abf8bda0b5994c
    restore-keys: compiled-deps-Windows-9.0.1-2021-11-29T12-30-07Z-
  compiled-deps-Windows-9.0.1-
  compiled-deps-Windows-

C:\sr???? afaik that is the global cache dir for stack 🤦. The default cabal store dir in windows is ~.\AppData\cabal\store (well before the CABAL_DIR change)
I have to confirm it though


This pr embrace the use of CABAL_DIR (setting it to a default value common for all os's if it is not set) for set the cache paths
We can restore the use of steps.HaskEnvSetup.outputs.cabal-store when the value is fixed upstream.

@Anton-Latukha

Copy link
Copy Markdown
Collaborator

Yep.

I've seen C:\sr even during creation. Even remember looking into some sources (of action probably). Assumed C:\sr is "I guess it just a caching directory on Windows".

@Anton-Latukha

Anton-Latukha commented Dec 18, 2021

Copy link
Copy Markdown
Collaborator

Before workflows were configured:

- if: runner.os == 'Windows'
name: (Windows) Platform config
run: |
echo "CABAL_STORE_DIR=$SYSTEMDRIVE\\SR" >> $GITHUB_ENV
echo "CABAL_PKGS_DIR=~\\AppData\\cabal\\packages" >> $GITHUB_ENV

So the configuration of the cabal store directory has not changed.

@jneira

jneira commented Dec 18, 2021

Copy link
Copy Markdown
Member Author

Ok i was wrong, C:\sr is also used by chocolatey to set the cabal store:

https://github.com/Mistuke/CabalChoco/blob/9e68bad284fe604b51bae94c30329d83d7318a33/3.6.2.0/cabal/tools/chocolateyInstall.ps1#L343-L344

So steps.HaskEnvSetup.outputs.cabal-store is good and my hypothesis is not

The C:\sr thing of stack:

imagen

So not sure if using C:\sr for cabal is a good idea but 🤷

@Anton-Latukha thanks for investigating it

@jneira

jneira commented Dec 18, 2021

Copy link
Copy Markdown
Member Author

So maybe the change of CABAL_DIR has affected the store dir?

@Anton-Latukha

Anton-Latukha commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

You are right about Windows build, store is not configured right, the C:/sr setting does not work.

Windows builds indeed ran 40m-1h longer than other builds, I guessed that they just compile & run tests that long. (& I am biased to look at Linux builds & at macOS as soon they would have fast hardware).

https://github.com/haskell/haskell-language-server/runs/4571121606?check_suite_focus=true

Shows that it both gets the relevant cache hit & then builds the store. So I agree. That means that C:/sr probably not works, but %APPDATA%\cabal\store would.

Would look into setup action code.

@Anton-Latukha

Copy link
Copy Markdown
Collaborator

I also seem like rember setup action switching or considering ghcup for Windows.

@jneira

jneira commented Dec 19, 2021

Copy link
Copy Markdown
Member Author

I also seem like rember setup action switching or considering ghcup for Windows.

yeah there is an issue about but no action afaik

Would look into setup action code.

Afair it is still using chocolatey to install ghc and cabal. So maybe it worth to check the code of the chocolatey install here: https://github.com/Mistuke/CabalChoco/blob/master/3.6.2.0/cabal/tools/chocolateyInstall.ps1

The code i linked in the previous comment is:

  # If running on Github actions, configure the package to pick things up
  if (($null -ne $Env:GITHUB_ACTIONS) -and ("" -ne $Env:GITHUB_ACTIONS)) {
    # Update the path on github actions as without so it won't be able to find
    # cabal.
    echo "$cabal_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

    # We probably don't need this since choco itself is already on the PATH
    # But it won't hurt to make sure.
    $choco_bin = Join-Path $env:ChocolateyInstall "bin"
    echo "$choco_bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

    # New GHC Packages will add themselves to the PATH, but older ones don't.
    # So let's find which one the user installed and add them to the pathh.
    $files = get-childitem $binRoot -include ghc.exe -recurse

    foreach ($file in $files) {
      $fileDir = Split-Path "$file"
      echo "$fileDir" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
    }

    # Also set a global SR.
    UpdateCabal-Config "store-dir" "$($env:SystemDrive)\SR"
  }

So it tries to change the cabal store dir adding in the global cabal config

store-dir: C:\sr

@Anton-Latukha

Copy link
Copy Markdown
Collaborator

@Anton-Latukha

Copy link
Copy Markdown
Collaborator

Ok, sorry, it is 02:37 at my location. I would look at the situation anew tomorrow.

@jneira
jneira force-pushed the fix-windows-cache branch 2 times, most recently from c854db2 to c3c710c Compare December 19, 2021 14:32
@Anton-Latukha

Anton-Latukha commented Dec 19, 2021

Copy link
Copy Markdown
Collaborator

Also note that the sources get redownloaded.

It is while the setting to cache them was:

      - if: runner.os == 'Windows'
        name: (Windows) Platform config
        run: |
          echo "CABAL_PKGS_DIR=~\\AppData\\cabal\\packages" >> $GITHUB_ENV

(it is seen for example in https://github.com/haskell/haskell-language-server/runs/4574676443?check_suite_focus=true)

So we try to cache C:\sr - caching is not working properly & ~\AppData\cabal\packages sources cache also does not work. It may/probably two different problems thou.

Also during work, to restart the caching anew - you can advance the Hackage index (cabal in any case rolls back to the last nearby fixed state), that both would load old caches, but workflows would make sure new information is saved in the new cache.

As for example, with Windows, the case may be that not full cache was saved somehow & gets reused (it can happen during initial cache runs in PR, for example, test or bench seizing the cache key save slot before caching & so saving test/bench cache there. But: 1. merge to master regenerated the caches, 2. what we see is not test or bench cache.


So all which are mentioned above - are details. But they are refuted (the is enough implication to mostly refute them in my logic system).


It seems to be some Windows setup & GitHub CI-related thing. For example, does CI allows to override directories in the root of a disk, because overriding root dirs is a security concern. Maybe it saved stack files, but cabal files are elsewhere. Maybe chocolatey package changed something & we keep supplying old cache & its file structure does not match the new one.

@Anton-Latukha
Anton-Latukha marked this pull request as draft December 19, 2021 22:26

@Anton-Latukha Anton-Latukha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I reset the review status so I can be called again when it would be suitable to review.

Please, request me to look into the code again, when we would have what to dialog about in it.

So far I have no idea why caching does not work for Windows.

(I plan to try running choco in VM also, to understand what it does myself, then I would have the ability of a deeper look into the code of things)

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

I am investigating the cache issue in windows in this pr in my fork: jneira#58, branch https://github.com/jneira/haskell-language-server/tree/fix-windows-cache2

Some things i discovered:

 reading package config: C:\sr\ghc-9.0.1\package.db\StateVar-1.2.2-ce901b4ca958e681fb5e3348adb2896320608537.conf
  • But the build uses another hash:
In order, the following will be built:
............
 - StateVar-1.2.2-987753584b04a1cf66bb17e1120e295276ba27af (lib) (requires build)
.........

It seems to me that it is a cabal bug in windows only triggered in ci. Locally it works fine and dont rebuild deps.

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

I am gonna trace the keys used to compute the hash

@Anton-Latukha

Anton-Latukha commented Dec 20, 2021

Copy link
Copy Markdown
Collaborator

Yes. It can be as simple as constant cache inheritance.

During merge I've looked through cache builds that they started & we're saving cache.

In master only Nix & Caching builds are run. So Caching should built & saved caches. & We essentially use caches from the last cache save of last project file hash change.

Just inserting a space somewhere or advance Hackage index. It may fix it.

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

Yes. It can be as simple as constant cache inheritance.

During merge I've looked through cache builds that they started & we're saving cache.

In master only Nix & Caching builds are run. So Caching should built & saved caches. & We essentially use caches from the last cache save of last project file hash change.

Just inserting a space somewhere or advance Hackage index. It may fix it.

Yeah that will be the next step, invalidate cache keys prefixing them with v2- (the definitive ones could be using the hackage index) and see how behave the cache eviction triggering more builds in my fork

Otoh triggering two sucessive builds in the same pr should trigger the cache hit with the good lib hashes. However in the main repo succesive builds of the same pr seems to not work. Will use a controlled pr in my fork to confirm or discard that.

@Anton-Latukha

Anton-Latukha commented Dec 20, 2021

Copy link
Copy Markdown
Collaborator

Nota bene: GitHub in the pipeline has a feature to manage the caching storage, on release of it maintainers would be able to navigate the caching storage. So we would be able to go, remove the problematic block & it would regenerate by itself or by just rerun of master workflow. It would be actions/cache#632.

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

Ok i have the evidence of the direct cause of the issue. I've traced the package hash keys of the existing package hash and the new one for one depedency StateVar
After the build we have:

Trace keys for computing package StateVar hash

Existing entry in the store StateVar-1.2.2-ce901b4ca958e681fb5e3348adb2896320608537:

pkgid: StateVar-1.2.2
component: ComponentLib
src: 5e4b39da395656a59827b0280508aafdc70335798b50e5d6fd52596026251825
pkg-config-deps: 
deps: base-4.15.0.0, stm-2.5.0.0, transformers-0.5.6.2
compilerid: ghc-9.0.1
platform: x86_64-windows
stripped-exe: False
debug-info: 0
ghc-options: -haddock

Rebuilt StateVar-1.2.2-987753584b04a1cf66bb17e1120e295276ba27af:

pkgid: StateVar-1.2.2
component: ComponentLib
src: 5e4b39da395656a59827b0280508aafdc70335798b50e5d6fd52596026251825
pkg-config-deps: 
deps: base-4.15.0.0, stm-2.5.0.0, transformers-0.5.6.2
compilerid: ghc-9.0.1
platform: x86_64-windows
stripped-exe: False
debug-info: 0
extra-lib-dirs: C:\msys64\mingw64\lib
extra-include-dirs: C:\msys64\mingw64\include
ghc-options: -haddock

I think the $CABAL_DIR change also changed the $CABAL_DIR/config adding this entries

 extra-include-dirs: C:\msys64\mingw64\include
-- deterministic:
-- cid:
extra-lib-dirs: C:\msys64\mingw64\lib
-- extra-framework-dirs:
extra-prog-path: C:\ghcup\bin,
                 C:\cabal\bin,
                 C:\msys64\usr\bin,
                 C:\msys64\mingw64\bin

(Observe the C:\ghcup\bin entry)


Ok so invalidating the cache should fix it.

what could we do? To consider:

  • this changes are pretty rare and a normal cache invalidation should fix it
  • as we can see only 5 fields from the global config can change for now: stripped-exe, debug-info, extra-include-dirs, extra-lib-dirs, ghc-options.
    • the more likely to be changed are extra-include-dirs, extra-lib-dirs
  • cabal freeze does not take in account global config files (i just checked it)

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

I think the $CABAL_DIR change also changed the $CABAL_DIR/config adding this entries

However i dont see anything in the pr which could cause that change:

https://github.com/actions/virtual-environments/pull/4264/files#diff-9028c63a6998459164e88e67823c4141a409a4632c4ecc604fc9b5f1a8c3f0f5R41

should have the opposite effect as th 5th parameter of the ps script which changes from false to true is

    # Skip adjusting cabal.config with mingw paths
    [switch]$NoAdjustCabalConfig,

🤔

@jneira

jneira commented Dec 20, 2021

Copy link
Copy Markdown
Member Author

Well in any case those little changes to adapt the source cache to the new $CABAL_DIR are good imo

@jneira
jneira marked this pull request as ready for review December 20, 2021 22:36
@jneira
jneira requested a review from Anton-Latukha December 20, 2021 22:36
@Anton-Latukha Anton-Latukha added the merge me Label to trigger pull request merge label Dec 21, 2021
@Anton-Latukha

Copy link
Copy Markdown
Collaborator

This rebase check would run a while, caching update is still ongoing in master.

But be ready for the merge 🧑‍🔧

@mergify
mergify Bot merged commit 070f16f into haskell:master Dec 21, 2021
@jneira

jneira commented Dec 21, 2021

Copy link
Copy Markdown
Member Author

@Anton-Latukha

Anton-Latukha commented Dec 21, 2021

Copy link
Copy Markdown
Collaborator

It is because rebase was done before the cache was built in master.

So PR used old caches & run the build in parallel & saved the caches in parallel. But thankfully all that happens localized to the PR scope.

That is why I restarted builds in #2506 after caching was done in master.

Further - the caching rebuilds would be faster (the deps already cached properly, so it would run at speed of project build (also ~50 minutes faster then first run we had)) & after the cache is proper - waits of master rebuilts are not that important, further, an old caches are almost as good as new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge me Label to trigger pull request merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants