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

Add WSL #1081

Merged
merged 7 commits into from
Jul 9, 2020
Merged

Add WSL #1081

merged 7 commits into from
Jul 9, 2020

Conversation

dsame
Copy link
Contributor

@dsame dsame commented Jun 19, 2020

Description

New tool

Installing Windows Subsystem for Linux is turning on Windows Optional feature. Installation time: 1sec

Related issue: #50

Check list

  • Related issue / work item is attached
  • Changes are tested and related VM images are successfully generated

@dsame dsame marked this pull request as ready for review June 25, 2020 18:38
Copy link
Contributor

@miketimofeev miketimofeev left a comment

Choose a reason for hiding this comment

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

Let's add validation script as usual and add wsl to software report

Copy link
Contributor

@AlenaSviridenko AlenaSviridenko left a comment

Choose a reason for hiding this comment

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

I also think we should add a note to Readme about WSL

@dsame dsame requested a review from AlenaSviridenko June 26, 2020 07:10
@dsame dsame requested a review from maxim-lobanov July 1, 2020 08:20
@AlenaSviridenko
Copy link
Contributor

/azp run windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AlenaSviridenko
Copy link
Contributor

/azp run windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AlenaSviridenko
Copy link
Contributor

/azp run windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Manishearth
Copy link

I think our Windows builds started failing because of this, see rust-lang/rust#73566 (comment)

@Manishearth
Copy link

We're fixing it by explicitly setting the shell: rust-lang/rust#74406

@The-Compiler
Copy link

FWIW if you have some kind of test which relies on bash working when it's available (and which gets skipped if not), this change likely will break it. In pytest, we moved to instead checking bash --version output: pytest-dev/pytest#7520

@al-cheb
Copy link
Contributor

al-cheb commented Jul 20, 2020

Hello, @The-Compiler.
After installing WSL feature we have got another place which contains bash.exe - C:\Windows\System32\bash.exe. On Windows, the subprocess class uses the Windows CreateProcess() function. If the file name does not contain a directory path, the system searches for the executable file in the following sequence:

  1. The directory from which the application loaded.
  2. The current directory for the parent process.
    3. The 32-bit Windows system directory. Use the GetSystemDirectory function to get the path of this directory.
  3. The 16-bit Windows system directory. There is no function that obtains the path of this directory, but it is searched. The name of this directory is System.
  4. The Windows directory. Use the GetWindowsDirectory function to get the path of this directory.
  5. The directories that are listed in the PATH environment variable. Note that this function does not search the per-application path specified by the App Paths registry key. To include this per-application path in the search sequence, use the ShellExecute function.

If the file name does not contain an extension, .exe is appended, therefore bash is equivalent to bash.exe . Using logic above it first looks in the GetSystemDirectory = C:\Windows\System32 directory , before than in the PATH environment variable.

In that case you should use follow approaches:

  1. Add the parameter shell=True - https://bugs.python.org/issue8557, https://stackoverflow.com/questions/24789749/popen-with-conflicting-executable-path
    subprocess.run(..., shell=True)

  2. Set a fully qualified path to a bash shell application:
    subprocess.run(['C:\\Program Files\\Git\\bin\\bash.exe']....)

  3. shutil. which() method tells the path to an executable application which would be run if the given cmd was called. This method can be used to find a file on computer which is present on the PATH.

  4. Delete C:\Windows\System32\bash.exe (@H-M-H)

umarcor added a commit to umarcor/ghdl-cosim that referenced this pull request Jul 21, 2020
@umarcor
Copy link

umarcor commented Jul 21, 2020

@The-Compiler I found the same issue with GitHub Actions and pytest. The following would fail:

    def setUp(self):
        self.shell = ['bash'] if platform == 'win32' else []
        print('\n::group::Log')
        sys.stdout.flush()

    def tearDown(self):
        print('\n::endgroup::')
        sys.stdout.flush()

    def _sh(self, args):
        check_call(self.shell + args, stderr=STDOUT)

Instead of setting shell=True or hardcoding the full path, I could work around it using which from shutil:

    def setUp(self):
        self.shell = [which('bash')] if platform == 'win32' else []
        print('\n::group::Log')
        sys.stdout.flush()

This seems to look into the PATH, skipping the GetSystemDirectory function that @al-cheb commented.

bluetech added a commit to bluetech/libxkbcommon that referenced this pull request Jul 22, 2020
The CI started installing some wrapper instead of a real bash which is
what gets found.

See:
actions/runner-images#1081

Note: the manual `run_command` can be replaced with `find_program`
once/if we depend on meson>=0.52.0 which added the `version` parameter.

Signed-off-by: Ran Benita <ran@unusedvar.com>
bluetech added a commit to bluetech/libxkbcommon that referenced this pull request Jul 22, 2020
The CI started installing some wrapper instead of a real bash which is
what gets found.

See:
actions/runner-images#1081

Given meson is written in python, it should always be available
hopefully.

Signed-off-by: Ran Benita <ran@unusedvar.com>
bluetech added a commit to bluetech/libxkbcommon that referenced this pull request Jul 22, 2020
The CI started installing some wrapper instead of a real bash which is
what gets found.

See:
actions/runner-images#1081

Given meson is written in python, it should always be available
hopefully.

Disabled valgrind wrapper for now because it now also applies to the
python interpreter which leaks like a sieve.

Signed-off-by: Ran Benita <ran@unusedvar.com>
bluetech added a commit to xkbcommon/libxkbcommon that referenced this pull request Jul 22, 2020
The CI started installing some wrapper instead of a real bash which is
what gets found.

See:
actions/runner-images#1081

Given meson is written in python, it should always be available
hopefully.

Disabled valgrind wrapper for now because it now also applies to the
python interpreter which leaks like a sieve.

Signed-off-by: Ran Benita <ran@unusedvar.com>
jordwalke pushed a commit to reasonml/reason that referenced this pull request Jul 26, 2020
* Fixes Windows CI: prepend '/c/Program Files/Git/bin'

Bash@3 started failing after a recent change related to WSL

actions/runner-images#1081
actions/runner-images#1276

* Fix slashes

* Pick bash from env
H-M-H added a commit to H-M-H/Weylus that referenced this pull request Jul 26, 2020
actions/runner-images#1081 introduced WSL
with a useless bash placeholder which sometimes shadows the actual bash
executable. Delete it!
@H-M-H
Copy link

H-M-H commented Jul 26, 2020

Another alternative that makes CreateProcess work again is to just delete: C:\Windows\System32\bash.exe

woeps pushed a commit to woeps/hello-reason that referenced this pull request Aug 10, 2020
Cherry-Picked from
reasonml/reason@d3822b7:
Fixes Windows CI: prepend '/c/Program Files/Git/bin' (#2611)

* Fixes Windows CI: prepend '/c/Program Files/Git/bin'

Bash@3 started failing after a recent change related to WSL

actions/runner-images#1081
actions/runner-images#1276

* Fix slashes

* Pick bash from env
jordwalke pushed a commit to esy-ocaml/hello-reason that referenced this pull request Aug 10, 2020
Cherry-Picked from
reasonml/reason@d3822b7:
Fixes Windows CI: prepend '/c/Program Files/Git/bin' (#2611)

* Fixes Windows CI: prepend '/c/Program Files/Git/bin'

Bash@3 started failing after a recent change related to WSL

actions/runner-images#1081
actions/runner-images#1276

* Fix slashes

* Pick bash from env
Et7f3 pushed a commit to Et7f3/hello-reason that referenced this pull request Sep 5, 2020
Cherry-Picked from
reasonml/reason@d3822b7:
Fixes Windows CI: prepend '/c/Program Files/Git/bin' (#2611)

* Fixes Windows CI: prepend '/c/Program Files/Git/bin'

Bash@3 started failing after a recent change related to WSL

actions/runner-images#1081
actions/runner-images#1276

* Fix slashes

* Pick bash from env
TomasHubelbauer added a commit to TomasHubelbauer/github-actions-wsl that referenced this pull request Apr 15, 2022
evacchi added a commit to evacchi/gha-playground that referenced this pull request Jan 26, 2023
evacchi added a commit to evacchi/gha-playground that referenced this pull request Jan 26, 2023
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.

9 participants