Expose ghcup binary to PATH on windows #4264

hasufell · 2021-10-12T17:14:09Z

The bootstrap-haskell.ps1 script uses
[System.EnvironmentVariableTarget]::User instead of
[System.EnvironmentVariableTarget]::Machine, so it appears
ghcup env vars and PATH update never make it. Do these manually
for now.

@newhoggy

haskell/actions#70 (comment)

Can't really say if this works as expected, hence the newly added tests. I tried to be as conservative as possible, so not changing default cabal config directory.

The bootstrap-haskell.ps1 script uses '[System.EnvironmentVariableTarget]::User' instead of '[System.EnvironmentVariableTarget]::Machine', so it appears ghcup env vars and PATH update never make it. Do these manually for now.

The config adjustment usually includes adding msys2 directories, so cabal can find `pkg-config` and libraries, e.g.: + C: \ghcup\msys64\mingw64\bin + extra-include-dirs: C:\ghcup\msys64\mingw64\include + extra-lib-dirs: C:\ghcup\msys64\mingw64\lib - extra-prog-path: C:\cabal\bin + extra-prog-path: C:\ghcup\bin,

hasufell · 2021-10-12T18:07:54Z

This modifies the default cabal.config to include msys2 paths, so it can find pkg-config and libraries/includes.

miketimofeev · 2021-10-12T19:36:01Z

/azp run windows2016, windows2019, windows2022

azure-pipelines · 2021-10-12T19:36:21Z

Azure Pipelines successfully started running 3 pipeline(s).

jneira · 2021-10-12T20:09:52Z

images/win/scripts/Installers/Install-Haskell.ps1

+$msysPath = "C:\msys64"
+$ghcupPrefix = "C:\"
+$appdata = [Environment]::GetEnvironmentVariable('APPDATA', [System.EnvironmentVariableTarget]::Machine) 
+$cabalDir = "$appdata\cabal"


I would try to honour a possible previous value of $CABAL_DIR just in case, as it is setting the env var in 45

there is no previous value I believe

jneira · 2021-10-12T20:23:28Z

This modifies the default cabal.config to include msys2 paths, so it can find pkg-config and libraries/includes.

I would consider not change it here by default; it could break some setups out there and workflows needing them will be already change it theirselves.

hasufell · 2021-10-12T20:26:57Z

I would consider not change it here by default; it could break some setups out there and workflows needing them will be already change it theirselves.

But it's the correct config that's done by chocolatey, haskell/actions and ghcup. I think people should expect the same config on the windows machine?

hasufell · 2021-10-12T20:31:32Z

A potential case that can cause problems is with posix-regex: https://gitlab.haskell.org/ghc/ghc/-/issues/19945
But you could argue that's an issue with posix-regex-clib and not a common case. The common case is: you want to be able to install system C libraries and use them in your haskell projects.

I'll think about it.

al-cheb · 2021-10-13T06:32:30Z

images/win/scripts/Installers/Install-Haskell.ps1

-Invoke-Command -ScriptBlock ([ScriptBlock]::Create($bootstrapHaskell)) -ArgumentList $false, $true, $true, $false, $false, $false, $false, C:\, "", C:\msys64, C:\cabal
+$msysPath = "C:\msys64"
+$ghcupPrefix = "C:\"
+$appdata = [Environment]::GetEnvironmentVariable('APPDATA', [System.EnvironmentVariableTarget]::Machine) 


AppData env var does not exist in Machine scope.

I'm having a hard time figuring out why env vars set for the user don't make it into the final image?

E.g. see https://gitlab.haskell.org/haskell/ghcup-hs/-/blob/f1cc2ebf204289c17cff5b6f0dafd49a71f9580c/scripts/bootstrap/bootstrap-haskell.ps1#L529

Seems we do need Machine scope?

And it seems my current code doesn't make it work either:

vhd: [-] GHCUP_INSTALL_BASE_PREFIX environment variable exists 866ms (667ms|200ms) vhd: Expected $true, but got $false. vhd: at [Environment]::GetEnvironmentVariables("Machine").ContainsKey($envVar) | Should -BeTrue, C:\image\Tests\Haskell.Tests.ps1:33 vhd: at <ScriptBlock>, C:\image\Tests\Haskell.Tests.ps1:33 vhd: [-] GHCUP_MSYS2 environment variable exists 26ms (25ms|1ms) vhd: Expected $true, but got $false. vhd: at [Environment]::GetEnvironmentVariables("Machine").ContainsKey($envVar) | Should -BeTrue, C:\image\Tests\Haskell.Tests.ps1:33 vhd: at <ScriptBlock>, C:\image\Tests\Haskell.Tests.ps1:33 vhd: [-] ghcup is installed 15ms (14ms|1ms) vhd: Command 'ghcup --version' has finished with exit code vhd: 'ghcup' is not recognized as an internal or external command, vhd: operable program or batch file. vhd: at "ghcup --version" | Should -ReturnZeroExitCode, C:\image\Tests\Haskell.Tests.ps1:58 vhd: at <ScriptBlock>, C:\image\Tests\Haskell.Tests.ps1:58

At the end of image generation process packer invokes sysprep util with /generalize param and removes the user which it uses during generation:
The Sysprep /generalize command removes unique information from a Windows installation so that you can reuse that image on different computers.

The value of %AppData% is C:\Users\USERNAME\AppData\Roaming <- That's why it exists only in the USER scope.

We should use the hardcoded variant as before, for example: $cabalDir = "C:\cabal"

@hasufell, Could you please apply changes above? ^^^

@al-cheb sure, bet that won't fix the issue with ghcup not being in PATH

al-cheb · 2021-10-13T06:35:05Z

images/win/scripts/Tests/Haskell.Tests.ps1

+    It "cabal config was modified and exists" {
+        [Environment]::GetEnvironmentVariable('CABAL_DIR', [System.EnvironmentVariableTarget]::Machine) | Should -Exist
+        "cabal user-config diff" | Should -Not -BeNullOrEmpty


It's just a string "cabal user-config diff". We should use ReturnZeroExitCode or &

Replace | Should -Not -BeNullOrEmpty to | Should -ReturnZeroExitCode

newhoggy · 2021-11-08T13:23:38Z

I take it once this PR is merged CABAL_DIR will be available to help identify where the cabal store will be?

For getting this directory before the PR is merged, I could use $APP_DATA/cabal?

hasufell · 2021-11-08T13:25:07Z

I take it once this PR is merged CABAL_DIR will be available to help identify where the cabal store will be?

For getting this directory before the PR is merged, I could use $APP_DATA/cabal?

The env vars I set don't seem to work and I don't really understand why: #4264 (comment)

miketimofeev · 2021-11-10T21:26:11Z

I take it once this PR is merged CABAL_DIR will be available to help identify where the cabal store will be?
For getting this directory before the PR is merged, I could use $APP_DATA/cabal?

The env vars I set don't seem to work and I don't really understand why: #4264 (comment)

@al-cheb thoughts on this?

images/win/scripts/Installers/Install-Haskell.ps1

images/win/scripts/Tests/Haskell.Tests.ps1

al-cheb · 2021-11-11T10:22:40Z

images/win/scripts/Tests/Haskell.Tests.ps1

+    It "cabal config was modified and exists" {
+        [Environment]::GetEnvironmentVariable('CABAL_DIR', [System.EnvironmentVariableTarget]::Machine) | Should -Exist
+        "cabal user-config diff" | Should -Not -BeNullOrEmpty


Replace | Should -Not -BeNullOrEmpty to | Should -ReturnZeroExitCode

images/win/scripts/Tests/Haskell.Tests.ps1

hasufell · 2021-11-18T22:34:49Z

Can we do a test run?

miketimofeev · 2021-11-19T14:29:38Z

/azp run windows2016, windows2019, windows2022

azure-pipelines · 2021-11-19T14:29:58Z

Azure Pipelines successfully started running 3 pipeline(s).

hasufell · 2021-11-19T17:13:37Z

The binary still cannot be found:

    vhd: Running tests from 'C:\image\Tests\Haskell.Tests.ps1'
    vhd: Describing Haskell
    vhd:   [+] GHCUP_INSTALL_BASE_PREFIX environment variable exists 456ms (251ms|205ms)
    vhd:   [+] GHCUP_MSYS2 environment variable exists 2ms (1ms|1ms)
    vhd:   [+] Accurate 3 versions of GHC are installed 480ms (477ms|4ms)
    vhd:   [+] GHC 8.10.7 is installed 48ms (44ms|3ms)
    vhd:   [+] GHC 9.0.1 is installed 31ms (30ms|1ms)
    vhd:   [+] GHC 9.2.1 is installed 41ms (40ms|1ms)
    vhd:   [+] GHC 9.2.1 is the default version and should be the latest installed 31ms (30ms|1ms)
    vhd:   [+] Cabal is installed 65ms (63ms|1ms)
    vhd:   [-] cabal config was modified and exists 60ms (58ms|2ms)
    vhd:    ParameterBindingValidationException: Cannot bind argument to parameter 'Path' because it is null.
    vhd:
    vhd:   [-] ghcup is installed 25ms (24ms|1ms)
    vhd:    Command 'ghcup --version' has finished with exit code
    vhd:        'ghcup' is not recognized as an internal or external command,
    vhd:        operable program or batch file.
    vhd:    at "ghcup --version" | Should -ReturnZeroExitCode, C:\image\Tests\Haskell.Tests.ps1:58
    vhd:    at <ScriptBlock>, C:\image\Tests\Haskell.Tests.ps1:58
    vhd: Tests completed in 4.67s
==> vhd: Test run has failed
    vhd: Tests Passed: 8, Failed: 2, Skipped: 0 NotRun: 0

I really don't understand these images.

al-cheb · 2021-11-30T13:55:29Z

images/win/scripts/Installers/Install-Haskell.ps1

+$msysPath = "C:\msys64"
+$ghcupPrefix = "C:\"
+$cabalDir = "C:\cabal"
+Invoke-Command -ScriptBlock ([ScriptBlock]::Create($bootstrapHaskell)) -ArgumentList $false, $true, $true, $false, $false, $false, $false, $ghcupPrefix, "", $msysPath, $cabalDir


@hasufell, This part of script is missing -> ScriptBlock ([ScriptBlock]::Create($bootstrapHaskell)) . The $bootstrapHaskell var is undefined.

@al-cheb done

al-cheb · 2021-12-01T14:43:44Z

images/win/scripts/Installers/Install-Haskell.ps1

@@ -35,5 +35,13 @@ Choco-Install -PackageName cabal
 Invoke-PesterTests -TestFile 'Haskell'


@hasufell, We should run test after installation. Could you please move this statement at the end?

@al-cheb done

hasufell · 2021-12-01T18:09:37Z

Cool, it seems to have succeeded.

This may break setups.

hasufell · 2021-12-01T18:17:04Z

@jneira @newhoggy so I think the only potentially breaking change here now is that we set CABAL_DIR to "C:\cabal", which we didn't do before. Do you consider that controversial given that it can fix MAX_PATH issues?

jneira · 2021-12-02T05:42:10Z

I think the change makes sense per se. I suppose (and hope) it will not break the choco installation, used right now in Haskell setup action.

@al-cheb is it possible to add a note in the image release about the breaking change? Including the possible workaround (unset the env var f.e.) if possible

al-cheb · 2021-12-02T08:44:07Z

I think the change makes sense per se. I suppose (and hope) it will not break the choco installation, used right now in Haskell setup action.

@al-cheb is it possible to add a note in the image release about the breaking change? Including the possible workaround (unset the env var f.e.) if possible

We can create a small announce. Could you please provide additional info that we should add to mitigate the issue?

jneira · 2021-12-02T09:49:02Z

Well, the workaorund would be unset the env var CABAL_DIR to make the cabal build tool pick the default, as before this pr. Not sure if the ps command

[Environment]::SetEnvironmentVariable("CABAL_DIR", $null ,"Machine")

will work for all cases

miketimofeev · 2021-12-08T16:27:48Z

/azp run windows2016, windows2019, windows2022

azure-pipelines · 2021-12-08T16:28:10Z

Azure Pipelines successfully started running 3 pipeline(s).

hasufell · 2021-12-10T09:27:02Z

Thanks! Where are the announcements posted btw?

miketimofeev · 2021-12-10T09:42:04Z

@hasufell thank you for your contribution. We have decided that the changes are relatively safe and skipped creating the announcement.

jneira · 2021-12-10T11:28:39Z

Thanks! Where are the announcements posted btw?

each VM update is recorded/announced as GitHub releases

jneira · 2021-12-13T11:23:04Z

A new windows vm release was announced: https://github.com/actions/virtual-environments/releases/tag/win19/20211212.1
It includes this although there is no mention in the release

Expose ghcup binary to PATH on windows

8b1ede4

The bootstrap-haskell.ps1 script uses '[System.EnvironmentVariableTarget]::User' instead of '[System.EnvironmentVariableTarget]::Machine', so it appears ghcup env vars and PATH update never make it. Do these manually for now.

hasufell force-pushed the jospald/PR/expose-ghcup-path-windows branch from ea9c21a to 8b1ede4 Compare October 12, 2021 17:15

hasufell marked this pull request as draft October 12, 2021 17:16

hasufell force-pushed the jospald/PR/expose-ghcup-path-windows branch from 3118adb to 7a0460b Compare October 12, 2021 17:53

hasufell marked this pull request as ready for review October 12, 2021 17:55

Expose cabal dir to PATH as well

11f1eb0

jneira reviewed Oct 12, 2021

View reviewed changes

al-cheb reviewed Oct 13, 2021

View reviewed changes

hasufell mentioned this pull request Oct 13, 2021

[Windows] Integrate better with msys2 and cabal config haskell/actions#78

Open

al-cheb reviewed Nov 11, 2021

View reviewed changes

images/win/scripts/Installers/Install-Haskell.ps1 Outdated Show resolved Hide resolved

al-cheb reviewed Nov 11, 2021

View reviewed changes

hasufell added 2 commits November 11, 2021 13:42

Use Set-SystemVariable

193d22b

Fix tests

2eb265e

Use hardcoded cabal dir

a99dc64

al-cheb reviewed Nov 30, 2021

View reviewed changes

Fix bootstrap-haskell

e73f4a7

al-cheb reviewed Dec 1, 2021

View reviewed changes

hasufell added 2 commits December 1, 2021 15:55

Move test after installation

a0fae3f

Print ghcup installation message

4a76bc2

Don't adjust cabal.config (to include mingw paths)

354a537

This may break setups.

MaksimZhukov approved these changes Dec 10, 2021

View reviewed changes

al-cheb approved these changes Dec 10, 2021

View reviewed changes

miketimofeev merged commit 1fb7d12 into actions:main Dec 10, 2021

jneira mentioned this pull request Dec 18, 2021

Fix ci cache for windows haskell/haskell-language-server#2507

Merged

		@@ -35,5 +35,13 @@ Choco-Install -PackageName cabal
		Invoke-PesterTests -TestFile 'Haskell'

Expose ghcup binary to PATH on windows #4264

Expose ghcup binary to PATH on windows #4264

Conversation

hasufell commented Oct 12, 2021 • edited Loading

hasufell commented Oct 12, 2021

miketimofeev commented Oct 12, 2021

azure-pipelines bot commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneira commented Oct 12, 2021

hasufell commented Oct 12, 2021 • edited Loading

hasufell commented Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasufell Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy commented Nov 8, 2021

hasufell commented Nov 8, 2021

miketimofeev commented Nov 10, 2021

Choose a reason for hiding this comment

hasufell commented Nov 18, 2021

miketimofeev commented Nov 19, 2021

azure-pipelines bot commented Nov 19, 2021

hasufell commented Nov 19, 2021

al-cheb Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasufell commented Dec 1, 2021

hasufell commented Dec 1, 2021

jneira commented Dec 2, 2021

al-cheb commented Dec 2, 2021

jneira commented Dec 2, 2021

miketimofeev commented Dec 8, 2021

azure-pipelines bot commented Dec 8, 2021

hasufell commented Dec 10, 2021

miketimofeev commented Dec 10, 2021

jneira commented Dec 10, 2021

jneira commented Dec 13, 2021

hasufell commented Oct 12, 2021 •

edited

Loading

hasufell commented Oct 12, 2021 •

edited

Loading

hasufell commented Oct 12, 2021 •

edited

Loading

hasufell Oct 13, 2021 •

edited

Loading

al-cheb Nov 30, 2021 •

edited

Loading