Skip to content
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

[Bug] Windows - Install-Msys2.ps1 - remove msys2 p7zip / 7z package #916

Merged
merged 1 commit into from
May 22, 2020

Conversation

MSP-Greg
Copy link
Contributor

Description

New tool, Bug fixing, or Improvement? Bug

Due to conflicts with two installed versions

See #905

@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Due to conflicts with two installed versions
@MSP-Greg
Copy link
Contributor Author

Rebased (duh) and added two minor changes. Worked locally...

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 21, 2020

Reminder

Once MSYS2 has been rolled out, the current Windows Rubies can be replaced with standard installs.

The 'vendored' code in RubyInstaller2 Rubies will find the MSYS2 install, so there won't be an issues. JFYI, the code looks for an embedded MSYS2 first, then checks C:/msys64.

In the most recent release:
https://github.com/oneclick/rubyinstaller2/releases/tag/RubyInstaller-2.7.1-1

The current download, rubyinstaller-devkit-2.7.1-1-x64.exe, is 131 MB, while the standard install, rubyinstaller-2.7.1-1-x64.7z, is 11 MB. Given that there are four Rubies installed (2.4 thru 2.7), that should free up a lot of disk space, and remove a lot of duplicated files.

I'd do a PR for it, but I can't find the code...

EDIT: Thanks to everyone helping with adding mSYS2.

0200517.1 is now available, and a few Ruby repos are working fine with ruby/setup-ruby and MSP-Greg/setup-ruby-pkgs

@al-cheb
Copy link
Contributor

al-cheb commented May 21, 2020

@MSP-Greg , I think we don't need to remove p7zip to unblock only one particular customer case.

Let's figure out how many apps can raise the same issue in the msys64 directory:

PS > dir C:\msys64\usr\bin | measure | % count
778

We can easily reproduce the issue for example with make util:

runs-on: [windows-latest]
    steps:
    - name: Install Ruby toolchain
      uses: ruby/setup-ruby@v1
      with:
          ruby-version: "2.6.3"
    - name: make
      run: |
         which make
         make --version
      shell: bash 

m - Copy

Normal behavior:

runs-on: [windows-latest]
    steps:
    - name: make
      run: |
         which make
         make --version
      shell: bash

s - Copy

As I know you provided the request to change git -> bash to msys64 -> bash that can resolve this issue with correct initialization msys2 environment when we use bash shell.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 21, 2020

@al-cheb

Everything you said is true. I thought an exception for p7zip was warranted because:

  1. I've used MSYS2 locally for a few years, and I've never installed the package.
  2. It's an old version of 7z, which also implies other users of MSYS2 don't have a need for it. Current 7zip version is 19.00, p7zip is 16.02.
  3. 7z is a 'native' windows app, and there's already two copies on the image.

With Windows CI, there's always going to be issues with GNU dev/build tools. Or, I think 7z is in a different category than things like gcc, make, openssl, etc...

My POV is also biased from helping macOS/nix types get Windows CI working, which probably shows in how ruby/setup-ruby works with Windows.

Long story short, if you want to close, that's ok by me...

@eregon
Copy link

eregon commented May 21, 2020

The 7z from MSYS2 seems clearly broken though, right?

/c/msys64/usr/bin/7z: line 2: /usr/lib/p7zip/7z: No such file or directory
##[error]Process completed with exit code 127.

What causes that?
Is there a way to make it work or is it completely unusable? If the latter I'd argue it should not be part of the installed packages.

Why does make fail above? I understand it will pick MSYS2's make, but shouldn't that make work too?

IIRC it seems most software already has to care to use make.exe explicitly on Windows to not get some half-broken gmake (by setting the MAKE env var). Which still seems unfortunate and confusing.

Since GitHub Actions is adding a MSYS2 installation, I would assume adding it to PATH should work reasonably well. If basic tools like 7z or make fail no matter the arguments it seems we need a way to fix that. AFAIK we had no such issues with the builtin Ruby MSYS2 DevKits from RubyInstaller.

@MSP-Greg
Copy link
Contributor Author

@eregon

The problem is using the bash shell, which is from Git, it's not the MSYS2 bash shell. See actions/runner#497

@eregon
Copy link

eregon commented May 21, 2020

Because the Git Bash does some initialization that MSYS2 binaries don't like, or conversely those binaries need some setup that MSYS2 bash does?

Why don't we see this issue with the RubyInstaller DevKit (+ Git bash) but we see them with GitHub Actions MSYS2? Simply because it has more packages?

@MSP-Greg
Copy link
Contributor Author

Do you know of any repos using shell: bash in any workflow steps?

Puma, Ruby, RubyGems, & ruby-loco are all working fine? I had to update setup-ruby-pkgs because of a GPG key issue (ruby 2.4 OpenSSL 1.0.2), and it builds the OpenSSL gem as a test, also ok...

@MSP-Greg
Copy link
Contributor Author

BTW:

  1. I got the version number of the MSYS2 p7zip package locally, so it at least that much works.

  2. Action custom shells - They appear to not work, as I just tried. Googled it, and found an issue I filed that I forgot about: Windows - steps - custom shell runner#460. It seems to not be affected by changes to PATH by previous steps?

@eine
Copy link

eine commented May 22, 2020

As I know you provided the request to change git -> bash to msys64 -> bash that can resolve this issue with correct initialization msys2 environment when we use bash shell.

Note that MSYS2 provides three different shells: MSYS, MINGW64 and MINGW32. Replacing git -> bash with msys64 -> bash is not enough, unless it defaults to MINGW64.

Since GitHub Actions is adding a MSYS2 installation, I would assume adding it to PATH should work reasonably well.

Ref #908.

MSYS2 provides a built-in mechanism to either inherit the PATH or set a new "clean" env for the shell. For reference, path-type is an option in eine/setup-msys2 and it defaults to strict. eine/setup-msys2 reuses the "default installation" when update: true. Examples of how the action is used can be found in https://github.com/ghdl/ghdl/actions, https://github.com/ghdl/ghdl-cosim/actions or https://github.com/ghdl/setup-ghdl-ci/actions.

@eine
Copy link

eine commented May 22, 2020

@al-cheb, see also actions/toolkit#318.

@MSP-Greg
Copy link
Contributor Author

I made changes to the script and ran it on Actions. Tried to bullet-proof it, changed the output a bit, including the logging of packages installed.

See Actions step here

I can:

  1. Do nothing
  2. Push it here
  3. Remove the the removal of p7zip and open a new PR

@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

pacman --noconfirm -Sy pacman
pacman --noconfirm -Su
Write-Host "pacman -Sy --noconfirm --needed pacman"
pacman -Sy --noconfirm --needed pacman
Copy link

@eine eine May 22, 2020

Choose a reason for hiding this comment

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

@miketimofeev, @al-cheb, as discussed in #906 (comment), pacman should NOT be updated first.

Copy link
Contributor

@al-cheb al-cheb May 22, 2020

Choose a reason for hiding this comment

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

@eine, Thank you for providing details. I think we can include these changes to the PR. Or if we don't want to mix changes. I can prepare a separate PR.

Copy link

Choose a reason for hiding this comment

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

@MSP-Greg
Copy link
Contributor Author

I think we can include these changes to the PR.

I've got changes that I can push as a separate commit here, the changes I'm referring to were in the CI I mentioned above, see See Actions step here.

The file is at https://github.com/MSP-Greg/actions-msys2-pkgs/blob/master/Install-Msys2.ps1#L50-L55, highlighted are the changes /additions, along with multiple taskkill /f /fi "MODULES eq msys-2.0.dll" statements.

Note that additional changes were made to logging, especially the package lists.

In general, the main issue here is that MSYS2 got to a state where 'standard update' code did not work (which is fixed in 20200517)). I've been using MSYS2 for several years, and I think this is only the second time that has happened.

So, I'm not sure we can be certain that whatever code we use can be guaranteed to install/update MSYS2 at every moment in time in the future. I haven't looked to see if MSYS2 has code checking this, if not, I'll suggest/contribute there.

@eine, any recollection of past issues?

@al-cheb - Thanks, can you have a look at the links?

@al-cheb
Copy link
Contributor

al-cheb commented May 22, 2020

@MSP-Greg, Thank you for updates. I have run the Instal-Msys2.ps1 script on a local machine. It works as expected without pre-installed 7z.

123 - Copy

7-zip is not pre-installed:
7z - Copy

@MSP-Greg
Copy link
Contributor Author

@al-cheb

Should I move the changes here with another commit? Are you ok with the other changes (logging, taskkill, etc)?

It's Friday, be nice for all of us if we can resolve this today...

@al-cheb
Copy link
Contributor

al-cheb commented May 22, 2020

@MSP-Greg , LGTM. I think we can include all changes in this PR.

@maxim-lobanov, What do you think?

@eine
Copy link

eine commented May 22, 2020

@eine, any recollection of past issues?

@MSP-Greg, overall, I believe that whoever that will take care of maintaining MSYS2 support in this virtual environment, should watch the main repos of the project and should have close contact with the maintainers (through issues, chat or private channels).

As you said, updating MSYS2 does NOT fail often, but it is a rolling distribution, and it is expected to have issues more frequently than frozen distributions. This is specially so because the (your) decision was to auto-update the environment by default and to include additional packages, as opposed to providing a simple and stable solution. I already explained in our previous discussions that such an approach would imply complications in the maintenance.

To be precise, @lazka is working hard these last weeks, and the only mechanism to be aware of it is to get partially involved: msys2/MSYS2-packages#1962 (comment)

@MSP-Greg
Copy link
Contributor Author

I haven't pushed another commit, but below is patch file:

diff --git a/images/win/scripts/Installers/Install-Msys2.ps1 b/images/win/scripts/Installers/Install-Msys2.ps1
index 8c5f859..90971b4 100644
--- a/images/win/scripts/Installers/Install-Msys2.ps1
+++ b/images/win/scripts/Installers/Install-Msys2.ps1
@@ -1,12 +1,14 @@
 ################################################################################
 ##  File:  Install-Msys2.ps1
-##  Desc:  Install Msys2 and 64-bit gcc, cmake, & llvm (32-bit commented out)
+##  Desc:  Install Msys2 and 64 & 32 bit gcc, cmake, & llvm
 ################################################################################
 
 # References
 # https://github.com/msys2/MINGW-packages/blob/master/azure-pipelines.yml
 # https://packages.msys2.org/group/
 
+$dash = "—" * 40
+
 $origPath = $env:PATH
 $gitPath  = "$env:ProgramFiles\Git"
 
@@ -17,7 +19,7 @@ $msys2Uri = ((Invoke-RestMethod $msys2_release).assets | Where-Object {
 
 $msys2File = "$env:TEMP\msys2.tar.xz"
 
-# Download the latest msys2 x86_64
+# Download the latest msys2 x86_64, filename includes release date
 Write-Host "Starting msys2 download using $($msys2Uri.split('/')[-1])"
 (New-Object System.Net.WebClient).DownloadFile($msys2Uri, $msys2File)
 Write-Host "Finished download"
@@ -31,7 +33,7 @@ $env:PATH = "$gitPath\usr\bin;$gitPath\mingw64\bin;$origPath"
 $tar = "$gitPath\usr\bin\tar.exe"
 
 # extract tar.xz to C:\
-Write-Host "Starting msys2 extraction from $msys2FileU"
+Write-Host "Starting msys2 extraction"
 &$tar -xJf $msys2FileU -C /c/
 Remove-Item $msys2File
 Write-Host "Finished extraction"
@@ -39,55 +41,52 @@ Write-Host "Finished extraction"
 # Add msys2 bin tools folders to PATH temporary
 $env:PATH = "C:\msys64\mingw64\bin;C:\msys64\usr\bin;$origPath"
 
-Write-Host "bash pacman-key --init"
+Write-Host "`n$dash bash pacman-key --init"
 bash.exe -c "pacman-key --init 2>&1"
 
 Write-Host "bash pacman-key --populate msys2"
 bash.exe -c "pacman-key --populate msys2 2>&1"
 
-Write-Host "pacman -Sy --noconfirm --needed pacman"
-pacman -Sy --noconfirm --needed pacman
-pacman -Su --noconfirm
-
-# Force stop gpg-agent to continue installation
-Get-Process gpg-agent -ErrorAction SilentlyContinue | Stop-Process -Force
-
-Write-Host "pacman --noconfirm -Syyuu"
+Write-Host "`n$dash pacman --noconfirm -Syyuu"
 pacman.exe -Syyuu --noconfirm
-pacman.exe -Syuu --noconfirm
+taskkill /f /fi "MODULES eq msys-2.0.dll"
+Write-Host "`n$dash pacman --noconfirm -Syuu (2nd pass)"
+pacman.exe -Syuu  --noconfirm
+taskkill /f /fi "MODULES eq msys-2.0.dll"
 
-Write-Host "Install msys2 packages"
+Write-Host "`n$dash Install msys2 packages"
 pacman.exe -S --noconfirm --needed --noprogressbar base-devel compression
+taskkill /f /fi "MODULES eq msys-2.0.dll"
 
-Write-Host "Remove p7zip/7z package due to conflicts"
+Write-Host "`n$dash Remove p7zip/7z package due to conflicts"
 pacman.exe -R --noconfirm --noprogressbar p7zip
 
 # mingw package list
 $tools = "___clang ___cmake ___llvm  ___toolchain ___ragel"
 
 # install mingw64 packages
-Write-Host "Install mingw64 packages"
+Write-Host "`n$dash Install mingw64 packages"
 $pre = "mingw-w64-x86_64-"
 pacman.exe -S --noconfirm --needed --noprogressbar $tools.replace('___', $pre).split(' ')
 
 # install mingw32 packages
-Write-Host "Install mingw32 packages"
+Write-Host "`n$dash Install mingw32 packages"
 $pre = "mingw-w64-i686-"
 pacman.exe -S --noconfirm --needed --noprogressbar $tools.replace('___', $pre).split(' ')
 
 # clean all packages to decrease image size
-Write-Host "Clean packages"
+Write-Host "`n$dash Clean packages"
 pacman.exe -Scc --noconfirm
 
-Write-Host "Installed mingw64 packages"
-pacman.exe -Qs --noconfirm mingw-w64-x86_64-
+Write-Host "`n$dash Installed mingw64 packages"
+pacman.exe -Q | grep ^mingw-w64-x86_64-
 
-Write-Host "Installed mingw32 packages"
-pacman.exe -Qs --noconfirm mingw-w64-i686-
+Write-Host "`n$dash Installed mingw32 packages"
+pacman.exe -Q | grep ^mingw-w64-i686-
 
-Write-Host "Installed msys2 packages"
-pacman.exe -Qs --noconfirm
+Write-Host "`n$dash Installed msys2 packages"
+pacman.exe -Q | grep -v ^mingw-w64-
 
-Write-Host "MSYS2 installation completed"
+Write-Host "`nMSYS2 installation completed"
 
-exit 0
\ No newline at end of file
+exit 0

@MSP-Greg
Copy link
Contributor Author

@eine

See msys2/msys2-installer#5.

Re rolling vs release, etc, etc --

I work a lot with Ruby master. Given that, there is an interest in always using the most recent gcc available. The most recent packages are zst files. Without a full MSYS2 update, those can't even be installed. One example, but there are also security updates (openssl, etc) and other examples.

Also, as I've mentioned before, the home page of msys.org shows updating as part of the normal user install process. We disagree on this, but we can certainly work together and benefit. Ok?

@maxim-lobanov
Copy link
Contributor

@MSP-Greg let's merge this PR for now to include it to next week deployment.
And we can make one more PR with remain changes.
Any concerns / objections?

@eine
Copy link

eine commented May 22, 2020

@MSP-Greg, sure! I believe that tight integration between upstream MSYS2, this repo and setup-msys2 (or any similar Action) is required in order to provide a pleasant UX. Since we are users, we'd better work together to benefit all of us! Let's move the discussion about higher-level strategies to msys2/msys2-installer#5.

@MSP-Greg
Copy link
Contributor Author

@maxim-lobanov

Just to clarify:

Any concerns / objections?

What, me worry?

Sorry. No, progress is good, and thanks.

And we can make one more PR with remain changes.

So the patch file above I'll open a new PR? Not sure if it will conflict with this, but I can wait until this is accepted...

@maxim-lobanov maxim-lobanov merged commit d7a86d8 into actions:master May 22, 2020
@MSP-Greg MSP-Greg deleted the win-msys2-p7zip branch May 22, 2020 17:30
@MSP-Greg
Copy link
Contributor Author

@maxim-lobanov

Thanks. See #928

@eine
Copy link

eine commented May 28, 2020

Ref #355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants