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

fix: pass through unexpected format strings #884

Closed
wants to merge 2 commits into from

Conversation

henryiii
Copy link
Contributor

Closes #840.

@henryiii henryiii closed this Oct 22, 2021
@henryiii henryiii reopened this Oct 22, 2021


class SafeDict(Dict[str, PathOrStr]):
def __missing__(self, key: str) -> LimitedExpandStr:
Copy link
Contributor Author

@henryiii henryiii Oct 22, 2021

Choose a reason for hiding this comment

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

Suggested change
def __missing__(self, key: str) -> LimitedExpandStr:
def __missing__(self, key: PathOrStr) -> LimitedExpandStr:

Will commit this after review, just to possibly save a CI run.

cibuildwheel/util.py Show resolved Hide resolved
def __format__(self, fmt: str) -> str:
return str(self.__class__(self.inner + (f":{fmt}" if fmt else "")))

def __getitem__(self: T, item: int) -> T:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __getitem__(self: T, item: int) -> T:
def __getitem__(self: T, item: Any) -> T:

T = TypeVar("T", bound="LimitedExpandStr")


class LimitedExpandStr:
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't honestly say I understand what's happening in this class... I'd have expected that

def __missing__(self, key):
    return "{" + key + "}"

would have done the job. what extra is this class doing? (might be nice to add this as a docstring, too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because valid Bash syntax will trip this up otherwise. This makes it slightly less brittle. Colons and especially brackets are valid in Bash, but would trip up our expansion if we didn't have this little class.

@joerick
Copy link
Contributor

joerick commented Oct 23, 2021

Ahhh. Been playing with this some more and just noticed another gotcha with prepare_command:

>>> import cibuildwheel.util
>>> cibuildwheel.util.prepare_command('find . -exec echo {} \;')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/joerick/Projects/cibuildwheel/cibuildwheel/util.py", line 88, in prepare_command
    return format_safe(command, python="python", pip="pip", **kwargs)
  File "/Users/joerick/Projects/cibuildwheel/cibuildwheel/util.py", line 78, in format_safe
    return template.format_map(FormatSafeDict(**kwargs))
ValueError: Format string contains positional fields

This format_map solution can't passthrough {} because it doesn't support positional arguments. Damn...

@joerick
Copy link
Contributor

joerick commented Oct 23, 2021

But... I remember the logic... that isn't a problem because users can do {{}}, which passes through fine. And ${} is not a bash construct, so it wouldn't conflict with GHA. Okay, that's fine then!

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.

Do not use Python's str.format() for expanding placeholders in commands
2 participants