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

[docs] Make shell interpreter explicit #1779

Open
JP-Ellis opened this issue Mar 7, 2024 · 5 comments
Open

[docs] Make shell interpreter explicit #1779

JP-Ellis opened this issue Mar 7, 2024 · 5 comments

Comments

@JP-Ellis
Copy link

JP-Ellis commented Mar 7, 2024

Summary

The docs should make it clear what shell is used when running commands such as before-build, before-all, before-test, etc.

Motivation

cibuildwheel allows for an arbitrary shell script to be executed in many places. The problem here is that it is unclear what shell is actually being used.

Naively, I would expect that:

  • Linux builds use /bin/sh or /bin/bash.
  • macOS builds use /bin/zsh as that is the default shell for macOS, though I would not be surprised if /bin/bash was used for consistency with Linux.
  • Windows build use either a Windows bash.exe for consistency, or the native cmd.exe or pwsh.exe.

I would rather avoid the trial-and-error, and the docs should make it really clear what shell is used.

Build log

No response

CI config

No response

@henryiii
Copy link
Contributor

henryiii commented Mar 7, 2024

This is just subprocess.run with shell=True. Python respects the $SHELL environment variable on Posix. Generally, this is whatever you are running on macOS. On Linux, since it runs in the manylinux docker container, you'll have bash. Not sure if it's always cmd on Windows or if it respects what you run it from.

In general, it's probably best to keep this simple and cross-platform, and then manually run a bash/bat/whatever script with complex commands if you need them. Or a Python script. :)

@JP-Ellis
Copy link
Author

JP-Ellis commented Mar 7, 2024

Yeah, I had a hunch it would just be calling subprocess.run. I guess for me, it just wasn't clear and other than trial and error, it was difficult to figure out.

I think there's a few things that could help here:

  1. Improving the docs to make what you explained really clear.
  2. Perhaps adding some clearer diagnostics in the CI logs that show which shell interpreter is being used.

Ultimately, it would be awesome to be able to specify the interpreter to allow something like:

before-build = {
  shell = "python",
  script = 'import shutil; shutil.rm("{package}/bin")'
}

Though I'm not sure if there's a desire for that, and it might complicate things significantly.

@henryiii
Copy link
Contributor

henryiii commented Mar 7, 2024

You can do that today. Use

before-build = [
 """python -c 'import shutil; shutil.rm("{package}/bin")'""",
]

Same for bash, etc. Better yet, stick the contents in a file, stick #!/usr/bin/env python (or bash, etc) at the top, make it executable, then run the file, like

before-build = [
 "./tools/remove_bin.py",
]

These are all cross platform and don’t depend on the shell Python uses.

Also, your example was invalid TOML (though it will be valid in TOML 1.1 eventually). Inline tables must be one line in 1.0. :(

@henryiii
Copy link
Contributor

henryiii commented Mar 7, 2024

(Not disagreeing with better docs, btw)

@joerick
Copy link
Contributor

joerick commented Mar 7, 2024

Agreed on the docs. It's one of those things that was implied and never said.

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

No branches or pull requests

3 participants