Skip to content

Commit

Permalink
Default to forward slash-separated paths for path translation (#2145)
Browse files Browse the repository at this point in the history 
## Changes

This came up in #2122 where relative library paths showed up with
backslashes on Windows. It's hard to run acceptance tests where paths
may be in either form. This change updates path translation logic to
always use forward slash-separated paths, including for absolute paths.

## Tests

* Unit tests pass.
* Confirmed that code where library paths are used uses the `filepath`
package for path manipulation. The functions in this package always
normalize their inputs to be platform-native paths.
* Confirmed that code that uses absolute paths works with forward
slash-separated paths on Windows.
  • Loading branch information
@pietern
pietern authored Jan 17, 2025
1 parent 2cd0d88 commit 9061635
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 38 deletions.
38 changes: 21 additions & 17 deletions bundle/config/mutator/translate_paths.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func (t *translateContext) rewritePath(
}

// Local path is relative to the directory the resource was defined in.
localPath := filepath.Join(dir, filepath.FromSlash(input))
localPath := filepath.Join(dir, input)
if interp, ok := t.seen[localPath]; ok {
return interp, nil
}
Expand All @@ -151,6 +151,10 @@ func (t *translateContext) rewritePath(
return "", fmt.Errorf("path %s is not contained in sync root path", localPath)
}

// Normalize paths to separated by forward slashes.
localPath = filepath.ToSlash(localPath)
localRelPath = filepath.ToSlash(localRelPath)

// Convert local path into workspace path via specified function.
var interp string
switch opts.Mode {
Expand Down Expand Up @@ -180,9 +184,9 @@ func (t *translateContext) rewritePath(
}

func (t *translateContext) translateNotebookPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) {
nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath))
nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, localRelPath)
if errors.Is(err, fs.ErrNotExist) {
if filepath.Ext(localFullPath) != notebook.ExtensionNone {
if path.Ext(localFullPath) != notebook.ExtensionNone {
return "", fmt.Errorf("notebook %s not found", literal)
}

Expand All @@ -198,7 +202,7 @@ func (t *translateContext) translateNotebookPath(ctx context.Context, literal, l
// way we can provide a more targeted error message.
for _, ext := range extensions {
literalWithExt := literal + ext
localRelPathWithExt := filepath.ToSlash(localRelPath + ext)
localRelPathWithExt := localRelPath + ext
if _, err := fs.Stat(t.b.SyncRoot, localRelPathWithExt); err == nil {
return "", fmt.Errorf(`notebook %s not found. Did you mean %s?
Local notebook references are expected to contain one of the following
Expand All @@ -218,42 +222,42 @@ to contain one of the following file extensions: [%s]`, literal, strings.Join(ex
}

// Upon import, notebooks are stripped of their extension.
localRelPathNoExt := strings.TrimSuffix(localRelPath, filepath.Ext(localRelPath))
return path.Join(t.remoteRoot, filepath.ToSlash(localRelPathNoExt)), nil
localRelPathNoExt := strings.TrimSuffix(localRelPath, path.Ext(localRelPath))
return path.Join(t.remoteRoot, localRelPathNoExt), nil
}

func (t *translateContext) translateFilePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) {
nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, filepath.ToSlash(localRelPath))
nb, _, err := notebook.DetectWithFS(t.b.SyncRoot, localRelPath)
if errors.Is(err, fs.ErrNotExist) {
return "", fmt.Errorf("file %s not found", literal)
}
if err != nil {
return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", localFullPath, err)
return "", fmt.Errorf("unable to determine if %s is not a notebook: %w", filepath.FromSlash(localFullPath), err)
}
if nb {
return "", ErrIsNotebook{localFullPath}
}
return path.Join(t.remoteRoot, filepath.ToSlash(localRelPath)), nil
return path.Join(t.remoteRoot, localRelPath), nil
}

func (t *translateContext) translateDirectoryPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) {
info, err := t.b.SyncRoot.Stat(filepath.ToSlash(localRelPath))
info, err := t.b.SyncRoot.Stat(localRelPath)
if err != nil {
return "", err
}
if !info.IsDir() {
return "", fmt.Errorf("%s is not a directory", localFullPath)
return "", fmt.Errorf("%s is not a directory", filepath.FromSlash(localFullPath))
}
return path.Join(t.remoteRoot, filepath.ToSlash(localRelPath)), nil
return path.Join(t.remoteRoot, localRelPath), nil
}

func (t *translateContext) translateLocalAbsoluteFilePath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) {
info, err := t.b.SyncRoot.Stat(filepath.ToSlash(localRelPath))
info, err := t.b.SyncRoot.Stat(localRelPath)
if errors.Is(err, fs.ErrNotExist) {
return "", fmt.Errorf("file %s not found", literal)
}
if err != nil {
return "", fmt.Errorf("unable to determine if %s is a file: %w", localFullPath, err)
return "", fmt.Errorf("unable to determine if %s is a file: %w", filepath.FromSlash(localFullPath), err)
}
if info.IsDir() {
return "", fmt.Errorf("expected %s to be a file but found a directory", literal)
Expand All @@ -262,12 +266,12 @@ func (t *translateContext) translateLocalAbsoluteFilePath(ctx context.Context, l
}

func (t *translateContext) translateLocalAbsoluteDirectoryPath(ctx context.Context, literal, localFullPath, _ string) (string, error) {
info, err := os.Stat(localFullPath)
info, err := os.Stat(filepath.FromSlash(localFullPath))
if errors.Is(err, fs.ErrNotExist) {
return "", fmt.Errorf("directory %s not found", literal)
}
if err != nil {
return "", fmt.Errorf("unable to determine if %s is a directory: %w", localFullPath, err)
return "", fmt.Errorf("unable to determine if %s is a directory: %w", filepath.FromSlash(localFullPath), err)
}
if !info.IsDir() {
return "", fmt.Errorf("expected %s to be a directory but found a file", literal)
Expand All @@ -281,7 +285,7 @@ func (t *translateContext) translateLocalRelativePath(ctx context.Context, liter

func (t *translateContext) translateLocalRelativeWithPrefixPath(ctx context.Context, literal, localFullPath, localRelPath string) (string, error) {
if !strings.HasPrefix(localRelPath, ".") {
localRelPath = "." + string(filepath.Separator) + localRelPath
localRelPath = "./" + localRelPath
}
return localRelPath, nil
}
Expand Down
4 changes: 2 additions & 2 deletions bundle/config/mutator/translate_paths_artifacts_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestTranslatePathsArtifacts_InsideSyncRoot(t *testing.T) {
require.NoError(t, diags.Error())

// Assert that the artifact path has been converted to a local absolute path.
assert.Equal(t, lib, b.Config.Artifacts["my_artifact"].Path)
assert.Equal(t, filepath.ToSlash(lib), b.Config.Artifacts["my_artifact"].Path)
}

func TestTranslatePathsArtifacts_OutsideSyncRoot(t *testing.T) {
Expand Down Expand Up @@ -79,5 +79,5 @@ func TestTranslatePathsArtifacts_OutsideSyncRoot(t *testing.T) {
require.NoError(t, diags.Error())

// Assert that the artifact path has been converted to a local absolute path.
assert.Equal(t, lib, b.Config.Artifacts["my_artifact"].Path)
assert.Equal(t, filepath.ToSlash(lib), b.Config.Artifacts["my_artifact"].Path)
}
2 changes: 1 addition & 1 deletion bundle/config/mutator/translate_paths_dashboards_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestTranslatePathsDashboards_FilePathRelativeSubDirectory(t *testing.T) {
// Assert that the file path for the dashboard has been converted to its local absolute path.
assert.Equal(
t,
filepath.Join(dir, "src", "my_dashboard.lvdash.json"),
filepath.ToSlash(filepath.Join(dir, "src", "my_dashboard.lvdash.json")),
b.Config.Resources.Dashboards["dashboard"].FilePath,
)
}
27 changes: 13 additions & 14 deletions bundle/config/mutator/translate_paths_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"os"
"path/filepath"
"runtime"
"strings"
"testing"

"github.com/databricks/cli/bundle"
Expand Down Expand Up @@ -226,7 +225,7 @@ func TestTranslatePaths(t *testing.T) {
)
assert.Equal(
t,
filepath.Join("dist", "task.whl"),
"dist/task.whl",
b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl,
)
assert.Equal(
Expand All @@ -251,7 +250,7 @@ func TestTranslatePaths(t *testing.T) {
)
assert.Equal(
t,
filepath.Join("dist", "task.jar"),
"dist/task.jar",
b.Config.Resources.Jobs["job"].Tasks[5].Libraries[0].Jar,
)
assert.Equal(
Expand Down Expand Up @@ -362,7 +361,7 @@ func TestTranslatePathsInSubdirectories(t *testing.T) {
)
assert.Equal(
t,
filepath.Join("job", "dist", "task.jar"),
"job/dist/task.jar",
b.Config.Resources.Jobs["job"].Tasks[1].Libraries[0].Jar,
)
assert.Equal(
Expand Down Expand Up @@ -774,8 +773,8 @@ func TestTranslatePathJobEnvironments(t *testing.T) {
diags := bundle.Apply(context.Background(), b, mutator.TranslatePaths())
require.NoError(t, diags.Error())

assert.Equal(t, strings.Join([]string{".", "job", "dist", "env1.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0])
assert.Equal(t, strings.Join([]string{".", "dist", "env2.whl"}, string(os.PathSeparator)), b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1])
assert.Equal(t, "./job/dist/env1.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0])
assert.Equal(t, "./dist/env2.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1])
assert.Equal(t, "simplejson", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[2])
assert.Equal(t, "/Workspace/Users/foo@bar.com/test.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[3])
assert.Equal(t, "--extra-index-url https://name:token@gitlab.com/api/v4/projects/9876/packages/pypi/simple foobar", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[4])
Expand Down Expand Up @@ -839,7 +838,7 @@ func TestTranslatePathWithComplexVariables(t *testing.T) {

assert.Equal(
t,
filepath.Join("variables", "local", "whl.whl"),
"variables/local/whl.whl",
b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl,
)
}
Expand Down Expand Up @@ -952,34 +951,34 @@ func TestTranslatePathsWithSourceLinkedDeployment(t *testing.T) {
// updated to source path
assert.Equal(
t,
filepath.Join(dir, "my_job_notebook"),
dir+"/my_job_notebook",
b.Config.Resources.Jobs["job"].Tasks[0].NotebookTask.NotebookPath,
)
assert.Equal(
t,
filepath.Join(dir, "requirements.txt"),
dir+"/requirements.txt",
b.Config.Resources.Jobs["job"].Tasks[2].Libraries[0].Requirements,
)
assert.Equal(
t,
filepath.Join(dir, "my_python_file.py"),
dir+"/my_python_file.py",
b.Config.Resources.Jobs["job"].Tasks[3].SparkPythonTask.PythonFile,
)
assert.Equal(
t,
filepath.Join(dir, "my_pipeline_notebook"),
dir+"/my_pipeline_notebook",
b.Config.Resources.Pipelines["pipeline"].Libraries[0].Notebook.Path,
)
assert.Equal(
t,
filepath.Join(dir, "my_python_file.py"),
dir+"/my_python_file.py",
b.Config.Resources.Pipelines["pipeline"].Libraries[2].File.Path,
)

// left as is
assert.Equal(
t,
filepath.Join("dist", "task.whl"),
"dist/task.whl",
b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl,
)
assert.Equal(
Expand All @@ -989,7 +988,7 @@ func TestTranslatePathsWithSourceLinkedDeployment(t *testing.T) {
)
assert.Equal(
t,
filepath.Join("dist", "task.jar"),
"dist/task.jar",
b.Config.Resources.Jobs["job"].Tasks[4].Libraries[0].Jar,
)
assert.Equal(
Expand Down
8 changes: 4 additions & 4 deletions bundle/tests/relative_path_with_includes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ func TestRelativePathsWithIncludes(t *testing.T) {
diags := bundle.Apply(context.Background(), b, m)
assert.NoError(t, diags.Error())

assert.Equal(t, filepath.Join(b.SyncRootPath, "artifact_a"), b.Config.Artifacts["test_a"].Path)
assert.Equal(t, filepath.Join(b.SyncRootPath, "subfolder", "artifact_b"), b.Config.Artifacts["test_b"].Path)
assert.Equal(t, b.SyncRootPath+"/artifact_a", b.Config.Artifacts["test_a"].Path)
assert.Equal(t, b.SyncRootPath+"/subfolder/artifact_b", b.Config.Artifacts["test_b"].Path)

assert.ElementsMatch(
t,
Expand All @@ -37,6 +37,6 @@ func TestRelativePathsWithIncludes(t *testing.T) {
b.Config.Sync.Exclude,
)

assert.Equal(t, filepath.Join("dist", "job_a.whl"), b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl)
assert.Equal(t, filepath.Join("subfolder", "dist", "job_b.whl"), b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl)
assert.Equal(t, "dist/job_a.whl", b.Config.Resources.Jobs["job_a"].Tasks[0].Libraries[0].Whl)
assert.Equal(t, "subfolder/dist/job_b.whl", b.Config.Resources.Jobs["job_b"].Tasks[0].Libraries[0].Whl)
}

0 comments on commit 9061635

Please sign in to comment.