Skip to content

Fix processing short pip flags in environment dependencies #3708

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

Merged
andrewnester merged 6 commits into from
Oct 3, 2025
Merged

Fix processing short pip flags in environment dependencies #3708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0bf34b0
Fix processing short pip flags in environment dependencies
andrewnester Oct 2, 2025
3013c11
changelog
andrewnester Oct 2, 2025
f62f9e3
handle local path
andrewnester Oct 2, 2025
5df9ec5
local to remote translation for pip flags
andrewnester Oct 2, 2025
fe23869
fix
andrewnester Oct 2, 2025
fa7327c
fixes
andrewnester Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEXT_CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@
### Dependency updates

### Bundles
* Fix processing short pip flags in environment dependencies ([#3708](https://github.com/databricks/cli/pull/3708))
Copy link
Contributor

@pietern pietern Oct 3, 2025

Choose a reason for hiding this comment

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

This PR now also handles relative paths for -e. We can include additional entries for the same PR, for different user-visible changes (eg call out -e specifically).

Copy link
Contributor Author

@andrewnester andrewnester Oct 3, 2025

Choose a reason for hiding this comment

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

Fixed here fa7327c


### API Changes
2 changes: 1 addition & 1 deletion bundle/libraries/local_path.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func IsLocalRequirementsFile(dep string) (string, bool) {
}

func containsPipFlag(input string) bool {
re := regexp.MustCompile(`--[a-zA-Z0-9-]+`)
re := regexp.MustCompile(`--?[a-zA-Z0-9-]+`)
Copy link
Contributor

@pietern pietern Oct 2, 2025

Choose a reason for hiding this comment

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

The caller of this function can now be simplified. It checks for -r right after.

Btw, how do we do local relative path interpretation for -r, and can we extend that to -e as well?

Copy link
Contributor Author

@andrewnester andrewnester Oct 2, 2025

Choose a reason for hiding this comment

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

@pietern fixed here f62f9e3
We do translation in TranslatePaths, added a comment about it

return re.MatchString(input)
}

Expand Down
1 change: 1 addition & 0 deletions bundle/libraries/local_path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func TestIsLibraryLocal(t *testing.T) {
{path: "s3://mybucket/path/to/package", expected: false},
{path: "dbfs:/mnt/path/to/package", expected: false},
{path: "beautifulsoup4", expected: false},
{path: "-e some/local/path", expected: false},
Copy link
Contributor

@lennartkats-db lennartkats-db Oct 2, 2025

Choose a reason for hiding this comment

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

There are some more tricky cases that we should cover here :-/ AI might help identify and handle them

-i http://myindexurl.com
--trusted-host pypi.org
--platform win_amd64
-e ${workspace.file_path}
-e /Workspace/path/to
-e /Volumes/path/to

Btw, Andrew, please consider how to incorporate such cases. It could be that what you have here is just net-better than what we have now and that we should have a followup with refinements. It could also be that we just need to revise this PR to cover things like -i properly, also to make sure we don't regress anything.

Copy link
Contributor Author

@andrewnester andrewnester Oct 2, 2025

Choose a reason for hiding this comment

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

We have quite a good coverage for this overall, but I've added some additional cases for unit / acceptacne tests


// Check the possible version specifiers as in PEP 440
// https://peps.python.org/pep-0440/#public-version-identifiers
Expand Down
Loading