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.

Sign up for GitHub

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

Jump to bottom

Allow specifying executable in artifact section and skip bash from WSL #1169

Merged
merged 6 commits into from
Feb 1, 2024
Merged

Allow specifying executable in artifact section and skip bash from WSL #1169

merged 6 commits into from
Feb 1, 2024

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Feb 1, 2024 •
edited
Loading

Changes

Allow specifying executable in artifact section

artifacts:
  test:
    type: whl
    executable: bash
    ...

We also skip bash found on Windows if it's from WSL because it won't be correctly executed, see the issue above

Fixes #1159

@andrewnester andrewnester requested a review from pietern February 1, 2024 11:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2024 •
edited
Loading

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (6beda44) 51.49% compared to head (37ad5c2) 51.45%.

Files Patch % Lines
libs/exec/exec.go 0.00% 13 Missing ⚠️
bundle/config/artifact.go 57.14% 1 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1169      +/-   ##
==========================================
- Coverage   51.49%   51.45%   -0.05%     
==========================================
  Files         292      292              
  Lines       16333    16352      +19     
==========================================
+ Hits         8411     8414       +3     
- Misses       7315     7329      +14     
- Partials      607      609       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewnester andrewnester changed the title Use CMD on Windows first as a shell to build artifacts Allow specifying executable in artifact section Feb 1, 2024
pietern
pietern reviewed Feb 1, 2024
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

What was the underlying issue in #1159?

libs/exec/exec.go Outdated Show resolved Hide resolved
@andrewnester
Copy link
Contributor Author

@pietern it's essentially as described in the ticket, when running deploy from let's say CMD, if CLI finds bash installed as WSL it tries to use it to build wheels. To execute scripts we create temporary files, in the case described in the issue they will be created from CMD scope in Windows tmp path. But when path to this script passed to found bash it can't be accessed because WSL bash does not have access to Windows FS paths.

@andrewnester
Copy link
Contributor Author

With this change, it's possible to explicitly tell CLI to use CMD to avoid the issue described in the ticket

@andrewnester andrewnester requested a review from pietern February 1, 2024 12:53
@pietern
Copy link
Contributor

pietern commented Feb 1, 2024

Understood. Then this fix allows Windows users to override and use cmd by specifying it, but the default will still be broken, correct? Can't we detect if the bash we resolve is WSL bash and then fall through? I.e. it's fine to use bash, as long as the CLI is also being executed from WSL. If it still OK to run bash if it is Git Bash or MingW Bash.

@andrewnester
Copy link
Contributor Author

Can't we detect if the bash we resolve is WSL bash and then fall through?

I haven't found reliable way yet, but I don't think we should because the logic would become a bit too complex as we will need to fall through only if WSL bash is detected but CLI is running NOT in WSL bash

@pietern
Copy link
Contributor

pietern commented Feb 1, 2024

I don't think it should be hard;

  • If we pick up Git or MINGW Bash, we can proceed as normal and use regular Windows paths
  • If we are running in WSL, we'll get Bash from /bin anyway, and the logic/behavior is no different from Linux
  • Therefore, as long as there is a predictable Windows-style path (or we use runtime.GOOS == "windows") that indicates we found the WSL trampoline, we can fall through and pick up cmd.

This also means the default templates work if WSL is installed. I suspect they don't right now because of this.

pietern
pietern approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Please hint in the summary that we're now skipping WSL bash when running on Windows.

libs/exec/shell_bash.go
@@ -33,5 +34,14 @@ func newBashShell() (shell, error) {
return nil, nil
}

// Skipping WSL bash if found one
if strings.Contains(out, `\Windows\System32\bash.exe`) || strings.Contains(out, `\Microsoft\WindowsApps\bash.exe`) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these can be suffix checks.

@andrewnester andrewnester changed the title Allow specifying executable in artifact section Allow specifying executable in artifact section and skip bash from WSL Feb 1, 2024
@andrewnester andrewnester added this pull request to the merge queue Feb 1, 2024
Merged via the queue into main with commit 0b3eeb8 Feb 1, 2024
4 checks passed
@andrewnester andrewnester deleted the windows-wsl-bash branch February 1, 2024 14:16
andrewnester added a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4
50c8752 
Bundles:
 * Allow specifying executable in artifact section and skip bash from WSL ([#1169](#1169)).
 * Added warning when trying to deploy bundle with `--fail-if-running` and running resources ([#1163](#1163)).
 * Group bundle run flags by job and pipeline types ([#1174](#1174)).
 * Make sure grouped flags are added to the command flag set ([#1180](#1180)).
 * Add short_name helper function to bundle init templates ([#1167](#1167)).

Internal:
 * Fix dynamic representation of zero values in maps and slices ([#1154](#1154)).
 * Refactor library to artifact matching to not use pointers ([#1172](#1172)).
 * Harden `dyn.Value` equality check ([#1173](#1173)).
 * Ensure every variable reference is passed to lookup function ([#1176](#1176)).
 * Empty struct should yield empty map in `convert.FromTyped` ([#1177](#1177)).
 * Zero destination struct in `convert.ToTyped` ([#1178](#1178)).
 * Fix integration test with invalid configuration ([#1182](#1182)).
 * Use `acc.WorkspaceTest` helper from bundle integration tests ([#1181](#1181)).
@andrewnester andrewnester mentioned this pull request Feb 7, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2024
@andrewnester
Release v0.212.4 (#1183)
f6cdc75 
Bundles:
* Allow specifying executable in artifact section and skip bash from WSL
([#1169](#1169)).
* Added warning when trying to deploy bundle with `--fail-if-running`
and running resources
([#1163](#1163)).
* Group bundle run flags by job and pipeline types
([#1174](#1174)).
* Make sure grouped flags are added to the command flag set
([#1180](#1180)).
* Add short_name helper function to bundle init templates
([#1167](#1167)).

Internal:
* Fix dynamic representation of zero values in maps and slices
([#1154](#1154)).
* Refactor library to artifact matching to not use pointers
([#1172](#1172)).
* Harden `dyn.Value` equality check
([#1173](#1173)).
* Ensure every variable reference is passed to lookup function
([#1176](#1176)).
* Empty struct should yield empty map in `convert.FromTyped`
([#1177](#1177)).
* Zero destination struct in `convert.ToTyped`
([#1178](#1178)).
* Fix integration test with invalid configuration
([#1182](#1182)).
* Use `acc.WorkspaceTest` helper from bundle integration tests
([#1181](#1181)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLI fails to deploy when building python wheels on Windows with WSL installed
3 participants
@andrewnester @codecov-commenter @pietern