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

Windows - Install-Msys2.ps1 - minor reliability, taskkill, logging #928

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

MSP-Greg
Copy link
Contributor

Description

New tool, Bug fixing, or Improvement? Improvement

  1. MSYS2 - add small changes to make MSYS2 installation/update via script more stable.

  2. Update logging code for both install/update and final package listing.

Tested both locally and with GHA

@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MSP-Greg
Copy link
Contributor Author

JFYI, Windows 2019 has C:\Windows\System32\tar.exe, Windows 2016 does not. When 2016 is dropped, code can be revised to remove dependency on Git tar. Seems messy to include conditional code for it now...

@maxim-lobanov maxim-lobanov mentioned this pull request May 25, 2020
5 tasks
@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 25, 2020

Ok, I'm confused. Azure Pipelines had all sorts of errors shown for the MSYS2 script.

I just ran the script in Actions (copied the ImageHelpers folder also) and it installed fine:
https://github.com/MSP-Greg/actions-msys2-pkgs/runs/707467166?check_suite_focus=true#step:3:610

I haven't used Azure Pipelines for a while, so ?

Happy to change whatever is needed...

@vsafonkin
Copy link
Contributor

vsafonkin commented May 26, 2020

Hi @MSP-Greg , the reason for the fails is this line:

$dash = "—" * 40

This dash is incorrect symbol.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 26, 2020

Apologies, encoding seemed suspect, but I didn't think Actions and Pipelines would be using different encodings. I recall wrestling with it on AppVeyor...

Fixed with a standard dash.

@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@MSP-Greg
Copy link
Contributor Author

It appears that the last CI failed before it got to Install-Msys2.ps1 ?

@miketimofeev
Copy link
Contributor

@MSP-Greg yeah, we're facing this issue at the moment
#940

@MSP-Greg
Copy link
Contributor Author

I pushed a change, adding the ~ folder (C:\msys64\home\$env:USERNAME). Kept forgetting about it, sorry...

@al-cheb
Copy link
Contributor

al-cheb commented May 27, 2020

I pushed a change, adding the ~ folder (C:\msys64\home\$env:USERNAME). Kept forgetting about it, sorry...

Please revert it. We can't preserver user name for different environments.

@MSP-Greg
Copy link
Contributor Author

@al-cheb

Please revert it

Please reconsider.

  1. It's only a folder

  2. MSYS2's normal shell sets it as the default for ~. Note that Git's shell uses /c/Users/$env:USERNAME.

  3. I suspect many of the installed apps write to HKCU, runneradmin appears in a few ENV variables, and appears twice in Path. Hence, USERNAME is kind of baked in anyway.

  4. Although most app's should check for a folder's existence, test scripts may not do so.

  5. This may be helpful when using cross-platform bash in CI.

  6. I'm the owner of MSP-Greg/setup-ruby-pkgs, and contributed to ruby/setup-ruby. In setup-ruby-pkgs, I've got code that adds this (~) folder. I didn't list a reason for doing so, but it must have been due to an issue, probably related to item 4. It really should be part of the install.

If you're adamant, I'll remove. Thanks.

@al-cheb
Copy link
Contributor

al-cheb commented May 27, 2020

@MSP-Greg, After completion image generation all users are delete from image.

@miketimofeev
Copy link
Contributor

@MSP-Greg as @al-cheb said this addition doesn't make sense, unfortunately. This runneradmin user is being added during VM creation only.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented May 27, 2020

@al-cheb miketimofeev

Ok. Removed the code.

Copy link
Contributor

@alepauly alepauly left a comment

Choose a reason for hiding this comment

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

Thanks @MSP-Greg! Just left a minor non-blocking question.

pacman.exe -Syyuu --noconfirm
pacman.exe -Syuu --noconfirm
taskkill /f /fi "MODULES eq msys-2.0.dll"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really necessary since we're installing?

Copy link

Choose a reason for hiding this comment

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

@alepauly, see #906 (comment). The procedure in this PR is different from the recommended in those references (-Syyuu vs -Syuu, and -Syuu vs -Suu). However, I think that this first execution of taskkill /f /fi "MODULES eq msys-2.0.dll" is required. I'm not sure about others, tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I'd err on the cautious side, as the script should be bullet-proof. I intend to keep an eye on future CI logs to see if all the taskkill statements are needed. Some were included based on doing the install in Actions (mult vm's on one system), which may not be the case for the Pipeliines runs...

Copy link

Choose a reason for hiding this comment

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

@MSP-Greg, I think that the first y is irrelevant (i.e. Syyuu or Syuu). However, I believe that using y in following statements is what might make taskkill to be required again. Anyway, this should be fixed upstream, albeit "experimental" (https://github.com/msys2/msys2-installer/releases/tag/2020-05-22). We should not need it in the near future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eine

Anyway, this should be fixed upstream

As mentioned above, 'the script should be bullet-proof'. If an extra few seconds are added to the install time to do that, so be it. As MSYS2 is tested better re scripted install, etc, the install script may be able to be simplified.

Both Ruby itself and several extension gems are running CI using the new MSYS2 install, and all are passing with no changes. All are using the gcc tools, the bash shell is used less frequently.

Copy link

Choose a reason for hiding this comment

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

Both Ruby itself and several extension gems are running CI using the new MSYS2 install, and all are passing with no changes. All are using the gcc tools, the bash shell is used less frequently.

In fact, most users of eine/setup-msys2 are using update: true. Hence, apart from the Ruby extensions gems, there is intensive testing going on. See msys2/setup-msys2#20. For example in ghdl/ghdl:actions nightly packages for MSYS2 are being built with setup-msys2.

@eine
Copy link

eine commented May 28, 2020

Ref #355

@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@maxim-lobanov maxim-lobanov merged commit 8c8f384 into actions:master Jun 2, 2020
@MSP-Greg MSP-Greg deleted the win-msys2-minor branch June 2, 2020 16:05
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