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
Do not use Python's str.format() for expanding placeholders in commands #840
Comments
Did you try to simply escape curly braces |
My first recommendation would be to set these in pyproject.toml if possible, then you avoid GitHub Actions expansion. (I was going to suggest |
|
YAML allows to escape special characters by putting line in |
I am using a |
How are you using
This would be bug prone, confusing to readers, and wouldn't allow us to add new expansions in the future. I'd still recommend placing your config in pyproject.toml. Possibly bundle the logic into a script and avoid complex multiline commands if possible. I don't think it's our fault for having a substitution mechanism. Might be more GitHub Action's fault if you want to blame someone. Honestly, I thought GHA used |
I am using Bash variables in the command, so I want the expansion on that level.
Thing is, there is no escape mechanism. If there were, I'd by happy, but the way I escape this makes it vulnerable on another level and vice versa. Also, there is not a single word in the docs that other placeholders in braces are also expanded, so users have to debug why they are getting random
|
if you have a multiline bash script with needs of escape then maybe wrap this in bash script which use arguments? |
That kind of defeats the purpose of my workflow file. The workflow file already litters my repository. I don't want to create additional shell scripts for every two lines of Bash code that I need for running my CI. In my specific case, I could simply remove the curly braces, because the character following the Bash variable happens to be a non-alpha character. But in general, it's still not a good solution. |
If you think that should be changed to avoid adding bash script files to the repository then maybe suggest another stable mechanism of text substitution? |
First, it should be documented that anything between braces will be expanded, which isn't obvious. Then there should be a stable way to escape braces that is compatible with YAML/GHA syntax. An alternative suggestion would be to do string replacement instead of format string parsing, which would replace only defined placeholders (imho, it's rather unlikely that a large number of additional placeholders will be added in the future that could break people's workflows). I could also live with some sort of signal character sequence at the beginning that turns off parameter substitution for the string, although that would imply that it's either a valid shell comment (difficult for Windows vs. Unix) or that the signal is stripped from the string before its passed on to the shell. |
You should not be using something like Windows doesn't have I feel like we already have a solution (the pyproject.yaml file instead of placing code in the CI file, which has less conflicting substitutions, and is more portable too), and custom rolling our own text replacement is likely to be tricky and error-prone (and again, WORSE in my opinion have We should be a bit clearer in the documentation, and maybe have an example of how to properly escape if anyone can think of a good use for
Why is using the proper bash syntax for variables not a good solution? With hundreds of cibulidwheel users, no one has complained about |
Both
Even though there is a
I sometimes use string replacements via Bash syntax, which is much shorter than piping something to
As I said, it is proper Bash syntax and if you look at various style guides, they even require them if you are using parameter substitution within a string of literal characters. |
I didn't say If we did have "smart" substitution that ignored missing names, what would you tell someone who expected this to work? export wheel=*.wheel
python -m pip install wheelhouse/${wheel} Depending on which command you are setting, this might be I'll let another maintainer chime in and see if they are interested in using a custom solution. If we used |
I doubt that users would expect anything from |
I'm not horribly against allowing |
For sure this is a sharp edge! Thank you for reporting it @phoerious. I'm surprised we have not hit this error sooner. I suppose some amount of nonsense is to be expected when the string processing pipeline looks like YAML -> Github actions -> cibuildwheel (str.format) -> bash. If it's possible, I'd rather not change our string expansion mechanism, because as henryiii noted, it introduces a (small) namespace issue, and could cause confusing error messages if a substitution was mistyped The least we can do is:
In terms of workarounds, what are we dealing with here? Apologies if I'm covering old ground, but I want to make sure I understand. So let's say the offending variable is
Then... I think we're out of options in terms of things that we can do inside the workflow file? The remaining options would be to move this config to pyproject.toml or write a bash script? |
As I've pointed out several times, there's also the option of moving the command to pyproject.toml, where To make it more fun, how about cookiecutter -> YAML -> Github actions -> cibuildwheel (str.format) -> bash, which I also do. :) |
Ah, yes, good point. I updated my previous post. But it would be nice if there was a way to do this with environment variables, too. And the TOML would still get the weird KeyError when doing |
You could add a custom escaping syntax with backslashes and translate that to double braces before handing it to string format. But I don't know how that would interact with YAML's own escape sequences. Worst case, you have to double-backslash everything. And yes, the TOML would be a workaround, but not a good one for three reasons:
|
@joerick, is there a reason these are not normal environment variables made available to the command via the shell? To make cross-platform easier? Yes, you'd still need to know that this is going to be processed with str.format, and double bracket to use brackets. |
This syntax goes back to the introduction of CIBW_BEFORE_BUILD, version 0.2.0 :) Yes, I'd assume that it was to make cross-platform config easier. And I didn't foresee GitHub Actions' expression syntax at the time!! :) |
That would have been impressive if you had, since it launched in 2019 and that's from 2017. :) Maybe we could add them to the environment with |
I haven't had the need for any substitutions other than input and output paths. Executables should be on the path, so there is no need for those. |
That's why |
But you are running them all in the same repo and with the same config files? Okay. But now GHA is literally the only place that can build your code correctly. Unless you duplicate the config, or use the local runner package that I've forgotten the name of (but keep meaning to play with). Oh, and you can set the file to a cibuildwheel specific toml file, you don't have to use pyproject.toml. You could have a central cibuildwheel.toml file and then point cibuildwheel at it for each package with
It's not CI backend code, it's a recipe for how to build your package. Ideally you can avoid any special setup, and just use pyproject.toml's python requirements, but if need command customization, then you can set things in pyproject.toml. And then you can build locally! Being able to run: pipx run cibuildwheel --platform linux locally is fantastic if your CI breaks down! Or if your ARM or PowerPC builds time out, Travis credits run out, etc. (All actually events that have happened, and where local builds were great). |
Two thoughts:
|
Maybe I found a solution. class Default(dict):
def __missing__(selfself, key):
return "{" + key + "}"
"text {aa} pr vc {bb}".format_map(Default(aa="eee"))
Out[7]: 'text eee pr vc {bb}' |
The map solution that you are both mentioning I've already brought up in #840 (comment):
Basically, nothing interesting on the bash side would work, like I'm not as against it as I was, though I also think most users should be avoiding complex scripting here anyway. |
PS: existing workarounds would break, |
No, but I might want to use the pre-defined environment variables in another context. Not every workflow looks like your basic hands-off setup where you only invoke the action formula and that's it. My workflow is much more complex and calling CIBW (multiple times) is just one part of it.
For me it definitely is. Running CIBW locally may be useful in some rare cases, but it's not the primary way to build the application, particularly since my workflow does a lot more than just building the wheels and some wheel build steps are specific to the CI environment and don't even work without the full CI workflow. Another solution to the parameter expansion problem perhaps: With the dict approach you could also do shell/environment variable expansion yourself by either replacing |
I did a little scan... Aside: this is a pretty cool tool, btw! It doesn't search all repos, but they say it's searching 2.1m repos, which is a lot :) I only found one use of the That said, if we used the str.format_map solution, I think the existing str.format escape mechanism would be preserved. So both string substitution and format_map seems like valid options to me. |
Yeah, I saw that, but didn't know you meant exactly the same since you said the special things wouldn't work and we'd need a fully custom rolled solution. What exactly did you mean?
Yes, that does seem to work, actually! Stealing the code from the SO answer (which is basically the same as @Czaki's):
|
Would such a thing (which seems backwards-compatible) actually work for @phoerious, or is such a change still missing something for you? |
I suppose it would work, but it needs to be documented clearly that these strings are parsed. |
The problem with format map is it still parses the strings, meaning the only thing it fixes is Oh, just tried it, and it actually allows these to be passed through. The only character that's not allowed is |
class Special:
def __init__(self, inner: str) -> None:
self.inner = inner
def __format__(self, fmt: str) -> str:
return str(self.__class__(self.inner + (f":{fmt}" if fmt else "")))
def __getitem__(self: T, item: int) -> T:
return self.__class__(f"{self.inner}[{item}]")
def __str__(self) -> str:
return f"{{{self.inner}}}"
class SafeDict(dict):
def __missing__(self, key):
return Special(key) >>> '{a,b}{b:.2e}{{c}}{d%s}{e:3}{f[0]}'.format_map(SafeDict({'a': 42, 'b': 3.14159}))
'{a,b}3.14e+00{c}{d%s}{e:3}{f[0]}' PS: I just discovered block select in iTerm. :D |
A parsed string would probably not allow for unmatched braces or other invalid Python format syntax, would it? |
Yes, that does seem to be the case, when I quickly test this. |
I suppose such expressions would be possible using I think the special format_map solution would be my preference. And yes, including a note about it in the docs. |
I guess unmatched |
Not if you quote the string: |
Only |
is invalid YAML. I guess what you could do is
|
The reason for this thread is that foo:
bar: "${baz}" is impossible to write currently. As written, it fails because Double brackets are fine in strings. This is not a thread on YAML syntax and strings - if you need a string, you should either be aware of the limitations of the shortcut allowing you to skip quotes, or you should use a quoting mechanism, like quotes or a block quote. Same issue if you try to use |
This was fixed in #889 |
Description
Placeholders in command environment variables such as
CIBW_BEFORE_BUILD
andCIBW_BEFORE_TEST
are expanded using Python'sstr.format()
:https://github.com/pypa/cibuildwheel/blob/main/cibuildwheel/util.py#L41
This makes it (nearly) impossible to use curly braces in a command, because a pair of single braces will throw a
KeyError
exception and a pair of double braces gets interpreted by the GitHub Actions YAML parser. The only option that might work is something ridiculous like{{ '{{' }}
.You should use a different parameter expansion mechanism that doesn't try to expand unknown parameter names.
Build log
No response
CI config
No response
The text was updated successfully, but these errors were encountered: