Allow specifying executable in artifact section and skip bash from WSL by andrewnester · Pull Request #1169 · databricks/cli

andrewnester · 2024-02-01T11:42:24Z

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

codecov-commenter · 2024-02-01T12:14:28Z

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.

pietern

What was the underlying issue in #1159?

andrewnester · 2024-02-01T12:50:04Z

@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 · 2024-02-01T12:52:36Z

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

pietern · 2024-02-01T13:01:59Z

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 · 2024-02-01T13:18:17Z

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 · 2024-02-01T13:36:45Z

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

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

pietern · 2024-02-01T14:02:41Z

 	}

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


Nit: these can be suffix checks.

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)).

#1169) ## 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

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)).

Use CMD on Windows first as a shell to build artifacts
299561d

andrewnester requested a review from pietern February 1, 2024 11:42

andrewnester added 2 commits February 1, 2024 12:12

Allow specifying executable in artifact section
ce69562

typo
37ad5c2

andrewnester changed the title ~~Use CMD on Windows first as a shell to build artifacts~~Allow specifying executable in artifact sectionFeb 1, 2024

pietern reviewed Feb 1, 2024
View reviewed changes

Comment threadlibs/exec/exec.go Outdated

added available options
8459cd9

andrewnester requested a review from pietern February 1, 2024 12:53

andrewnester added 2 commits February 1, 2024 13:52

skip WSL bash
2f8d048

set executable
b2e7702

pietern approved these changes Feb 1, 2024
View reviewed changes

andrewnester changed the title ~~Allow specifying executable in artifact section~~Allow specifying executable in artifact section and skip bash from WSLFeb 1, 2024

andrewnester added this pull request to the merge queue Feb 1, 2024

Merged via the queue into main with commit 0b3eeb8Feb 1, 2024

andrewnester deleted the windows-wsl-bash branch February 1, 2024 14:16

andrewnester mentioned this pull request Feb 7, 2024
Release v0.212.4 #1183
Merged

Navigation Menu

Uh oh!

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

Allow specifying executable in artifact section and skip bash from WSL#1169
andrewnester merged 6 commits into
mainfrom
windows-wsl-bash

andrewnester commented Feb 1, 2024•
edited
Loading

Uh oh!

codecov-commenter commented Feb 1, 2024•
edited
Loading

Uh oh!

pietern left a comment

Uh oh!

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

pietern commented Feb 1, 2024

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

pietern commented Feb 1, 2024

Uh oh!

pietern left a comment

Uh oh!

pieternFeb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

andrewnester commented Feb 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov-commenter commented Feb 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

pietern commented Feb 1, 2024

Uh oh!

andrewnester commented Feb 1, 2024

Uh oh!

pietern commented Feb 1, 2024

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

pieternFeb 1, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrewnester commented Feb 1, 2024•
edited
Loading

codecov-commenter commented Feb 1, 2024•
edited
Loading