-
Notifications
You must be signed in to change notification settings - Fork 92
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.
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
Allow to set ghcup msys2 environment #992
Conversation
6ec43f0
to
f682dbb
Compare
@hasufell how could we go about testing this? Are all those environments supposed to Just Work for any msys installation (excluding e.g. arm64 on x86, of course)? |
I don't think I'm gonna test functionality on any env but the default one. Testing whether setting the env works correctly will mostly amount to using something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm gonna test functionality on any env but the default one.
I really dislike testing in production. If we merge this change, how will we know it's actually working without some kind of test?
For example, it looks like you've forgotten to append /bin
to the paths returned by ghcupMsys2BinDirs'
. I would like to have a test that catches this.
As described in #982 all the different environments can have different issues when compiling and linking against C libraries. I don't think I have the bandwidth to setup proper tests for each of them. And I'm happy to mark using any env other than MINGW64 as experimental.
We have no tests for linking C libraries at the moment. It is too fragile, because it depends on the state of the msys2 repositories, which are rolling-release.
Yes, I know. I'm testing this manually in a VM. We can add the checks I described above to the test suite, e.g.:
|
ad86f76
to
0930c78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial questions. Will test in a VM as the next step, and leave another review based on that.
$curDir = Get-Location | ||
Write-Host "Current Working Directory: $curDir" | ||
./scripts/bootstrap/bootstrap-haskell.ps1 -InstallDir ${GITHUB_WORKSPACE} -BootstrapUrl ("{0}/scripts/bootstrap/bootstrap-haskell" -f $curDir) -InBash | ||
./scripts/bootstrap/bootstrap-haskell.ps1 -InstallDir ${GITHUB_WORKSPACE} -BootstrapUrl ("{0}/scripts/bootstrap/bootstrap-haskell" -f $curDir) -InBash -Msys2Env "MINGW64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the Windows/PowerShell installation command listed here (after clicking "Show all platforms") be changed if we default to passing -Msys2Env "MINGW64"
here?
Perhaps it would make sense to also test the default in CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the Windows/PowerShell installation command listed here (after clicking "Show all platforms") be changed if we default to passing -Msys2Env "MINGW64" here?
No, not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hasufell I think the Windows/PowerShell installation command listed on https://www.haskell.org/ghcup should be tested in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is:
ghcup-hs/.github/workflows/bootstrap.yaml
Line 57 in 2a79c66
./scripts/bootstrap/bootstrap-haskell.ps1 -InstallDir ${GITHUB_WORKSPACE} -BootstrapUrl ("{0}/scripts/bootstrap/bootstrap-haskell" -f $curDir) -InBash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... this PR proposes to change that line.
Furthermore, why pass that argument when it's the default anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you are proposing. Do you want to test all combinations of flags and their effects? That's not in scope for this PR and probably considerable work, given that it's just plain powershell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hasufell I'm proposing we test the command listed on https://www.haskell.org/ghcup, ie. not passing -Msys2Env
This probably needs to depend on the environment as well, specifically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to assess whether this works as intended since it's unclear to me exactly what the indented usage is of the new options and environment variables. #992 contains a lot of back-and-forth conversation, but I'm missing a more concise spec of what, exactly, this PR is implementing.
For example, what happens if I set GHCUP_MSYS2_ENV
to e.g. "CLANG64" when running GHCup but it was installed using scripts/bootstrap/bootstrap-haskell.ps1
without setting Msys2Env
? Also, see review comments with questions.
$curDir = Get-Location | ||
Write-Host "Current Working Directory: $curDir" | ||
./scripts/bootstrap/bootstrap-haskell.ps1 -InstallDir ${GITHUB_WORKSPACE} -BootstrapUrl ("{0}/scripts/bootstrap/bootstrap-haskell" -f $curDir) -InBash | ||
./scripts/bootstrap/bootstrap-haskell.ps1 -InstallDir ${GITHUB_WORKSPACE} -BootstrapUrl ("{0}/scripts/bootstrap/bootstrap-haskell" -f $curDir) -InBash -Msys2Env "MINGW64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... this PR proposes to change that line.
Furthermore, why pass that argument when it's the default anyway?
$Msys2EnvExport = ('export GHCUP_MSYS2_ENV={0} ;' -f $Msys2Env) | ||
$null = [Environment]::SetEnvironmentVariable("GHCUP_MSYS2_ENV", $Msys2Env, [System.EnvironmentVariableTarget]::User) | ||
} | ||
|
||
if ((Get-Process -ID $PID).ProcessName.StartsWith("bootstrap-haskell") -Or $InBash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused here as well about why this is done. Please add a code comment to clarify why this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hasufell here I was talking specifically about the line starting with
if ((Get-Process -ID $PID).ProcessName.StartsWith("bootstrap-haskell") -Or $InBash) {
and below. Why is this done?
I don't really understand what you're requesting. This PR implements support for MSYS2 environments as specified here: https://www.msys2.org/docs/environments/ Based on the environment picked by the user, we:
|
@hasufell I'm missing an answer to this question. EDIT: Ie. does the range of supported values for |
Two things happen:
The documentation of This is an advanced feature that will not be exposed to end-users through a question during bootstrap. I expect users to understand the implications. The rest could/should be done at cabal documentation (@Kleidukos). |
More than happy to continue the conversation on the cabal ticket tracker. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just installed GHCup in a Windows VM using this PR's version of scripts/bootstrap/bootstrap-haskell.ps1
and it seems setting GHCUP_MSYS2_ENV
has no effect. C:\ghcup\msys64\mingw64\bin
is added to the PATH regardless, and setting GHCUP_MSYS2_ENV
to an invalid value doesn't throw an error:
rune@RUNESVENDSEFBC3 MINGW64 ~
$ ghcup debug-info
[ Info ] verifying digest of: gs.exe
Debug Info
==========
GHCup base dir: C:\ghcup
GHCup bin dir: C:\ghcup\bin
GHCup GHC directory: C:\ghcup\ghc
GHCup cache directory: C:\ghcup\cache
Architecture: x86_64
Platform: Windows
Version: 0.1.20.0
rune@RUNESVENDSEFBC3 MINGW64 ~
$ echo $GHCUP_MSYS2_ENV
CLANG64
rune@RUNESVENDSEFBC3 MINGW64 ~
$ ghcup run -m -- echo $PATH
[ Warn ] New ghc version available. If you want to install this latest version, run 'ghcup install ghc 9.8.1'
[ Info ] verifying digest of: gs.exe
C:\Users\rune\.cabal\bin;C:\ghcup\bin;C:\ghcup\msys64\mingw64\bin;C:\ghcup\msys64\usr\local\bin;C:\ghcup\msys64\usr\bin;C:\ghcup\msys64\usr\bin;C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\ghcup\msys64\usr\bin\site_perl;C:\ghcup\msys64\usr\bin\vendor_perl;C:\ghcup\msys64\usr\bin\core_perl
rune@RUNESVENDSEFBC3 MINGW64 ~
$ export GHCUP_MSYS2_ENV=blah
rune@RUNESVENDSEFBC3 MINGW64 ~
$ echo $GHCUP_MSYS2_ENV
blah
rune@RUNESVENDSEFBC3 MINGW64 ~
$ ghcup run -m -- echo $PATH
[ Warn ] New ghc version available. If you want to install this latest version, run 'ghcup install ghc 9.8.1'
[ Info ] verifying digest of: gs.exe
C:\Users\rune\.cabal\bin;C:\ghcup\bin;C:\ghcup\msys64\mingw64\bin;C:\ghcup\msys64\usr\local\bin;C:\ghcup\msys64\usr\bin;C:\ghcup\msys64\usr\bin;C:\Windows\System32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\ghcup\msys64\usr\bin\site_perl;C:\ghcup\msys64\usr\bin\vendor_perl;C:\ghcup\msys64\usr\bin\core_perl
Also, a few more questions.
if ((Get-Process -ID $PID).ProcessName.StartsWith("bootstrap-haskell") -Or $InBash) { | ||
Exec "$Bash" '-lc' ('{4} {6} {7} {8} {9} {10} {12} [ -n ''{1}'' ] && export GHCUP_MSYS2=$(cygpath -m ''{1}'') ; [ -n ''{2}'' ] && export GHCUP_INSTALL_BASE_PREFIX=$(cygpath -m ''{2}/'') ; export PATH=$(cygpath -u ''{3}/bin''):$PATH ; export CABAL_DIR=''{5}'' ; [[ ''{0}'' = https* ]] && {11} {0} | bash || cat $(cygpath -m ''{0}'') | bash' -f $BootstrapUrl, $MsysDir, $GhcupBasePrefix, $GhcupDir, $SilentExport, $CabalDirFull, $StackInstallExport, $HLSInstallExport, $AdjustCabalConfigExport, $MinimalExport, $BootstrapDownloader, $DownloadScript, $AdjustBashRcExport) | ||
Exec "$Bash" '-lc' ('{4} {6} {7} {8} {9} {10} {12} {13} [ -n ''{1}'' ] && export GHCUP_MSYS2=$(cygpath -m ''{1}'') ; [ -n ''{2}'' ] && export GHCUP_INSTALL_BASE_PREFIX=$(cygpath -m ''{2}/'') ; export PATH=$(cygpath -u ''{3}/bin''):$PATH ; export CABAL_DIR=''{5}'' ; [[ ''{0}'' = https* ]] && {11} {0} | bash || cat $(cygpath -m ''{0}'') | bash' -f $BootstrapUrl, $MsysDir, $GhcupBasePrefix, $GhcupDir, $SilentExport, $CabalDirFull, $StackInstallExport, $HLSInstallExport, $AdjustCabalConfigExport, $MinimalExport, $BootstrapDownloader, $DownloadScript, $AdjustBashRcExport, $Msys2EnvExport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this line do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It runs the bootstrap haskell shellscript.
} else { | ||
Exec "$Msys2Shell" '-mingw64' '-mintty' '-shell' 'bash' '-c' ('{4} {6} {7} {8} {9} {10} {12} [ -n ''{1}'' ] && export GHCUP_MSYS2=$(cygpath -m ''{1}'') ; [ -n ''{2}'' ] && export GHCUP_INSTALL_BASE_PREFIX=$(cygpath -m ''{2}/'') ; export PATH=$(cygpath -u ''{3}/bin''):$PATH ; export CABAL_DIR=''{5}'' ; trap ''echo Press any key to exit && read -n 1 && exit'' 2 ; [[ ''{0}'' = https* ]] && {11} {0} | bash || cat $(cygpath -m ''{0}'') | bash ; echo ''Press any key to exit'' && read -n 1' -f $BootstrapUrl, $MsysDir, $GhcupBasePrefix, $GhcupDir, $SilentExport, $CabalDirFull, $StackInstallExport, $HLSInstallExport, $AdjustCabalConfigExport, $MinimalExport, $BootstrapDownloader, $DownloadScript, $AdjustBashRcExport) | ||
Exec "$Msys2Shell" $ShellType '-mintty' '-shell' 'bash' '-c' ('{4} {6} {7} {8} {9} {10} {12} {13} [ -n ''{1}'' ] && export GHCUP_MSYS2=$(cygpath -m ''{1}'') ; [ -n ''{2}'' ] && export GHCUP_INSTALL_BASE_PREFIX=$(cygpath -m ''{2}/'') ; export PATH=$(cygpath -u ''{3}/bin''):$PATH ; export CABAL_DIR=''{5}'' ; trap ''echo Press any key to exit && read -n 1 && exit'' 2 ; [[ ''{0}'' = https* ]] && {11} {0} | bash || cat $(cygpath -m ''{0}'') | bash ; echo ''Press any key to exit'' && read -n 1' -f $BootstrapUrl, $MsysDir, $GhcupBasePrefix, $GhcupDir, $SilentExport, $CabalDirFull, $StackInstallExport, $HLSInstallExport, $AdjustCabalConfigExport, $MinimalExport, $BootstrapDownloader, $DownloadScript, $AdjustBashRcExport, $Msys2EnvExport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this line do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also runs the bootstrap haskell shellscript.
This does not work. As the integration test suite shows: https://github.com/haskell/ghcup-hs/pull/992/files#diff-d2a2c2295ab9310565a95908a3da85cfac1fa1bc63dff49f34f6c049d099f71eR45 You have to do |
c6d6939
to
a8f257d
Compare
049a111
to
096ca54
Compare
Fixes #982