-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add the ability to declare safe tools in a cross-build environment. #2317
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @freakboy3742,
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! My main question is around defaults - I had expected that we'd set a 'sensible default' for this, including cmake. What's the thinking regarding requiring individual opt-ins?
cibuildwheel/resources/defaults.toml
Outdated
@@ -14,6 +14,7 @@ build-verbosity = 0 | |||
|
|||
before-all = "" | |||
before-build = "" | |||
safe-tools = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a default seemed "a bit too political" for @freakboy3742 in the previous PR if I recall correctly.
Do we want a default of ["cmake", "ninja", "swig", "rustc"]?
If we have a default then yes, that would have been my proposition.
I think that without a default, scikit-build-core won't work without configuration...
I think that without a default, it'll try to build cmake & fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a default of
["cmake", "ninja", "swig", "rustc"]
? We're pretty sure these tools do the right thing I think? cc @mayeut I think this was your original plan?
My thought here is essentially "explicit is better than implicit", for a number of reasons:
- If you bless those 4 tools, then someone says "what about ?" - you now have a project policy decision to make about what constitutes sufficient notability to be in the default set? For example - why
ninja
, but notmeson
? Similarly, is there a point at which a tool would be removed from the default set? - It means we'd have to remove (or at least modify) the "error if not found" handling. If someone tries to build a project that we know needs
cmake
, but they don't havecmake
in their environment, then it seems better to me to error out as early as possible, rather than get mid build and fail. Alternatively, @mayeut's suggestion about having the setting extend the default list, and not raise an error for the default list, means there's inconsistent behavior between builtin and default tools. - If there's a default set (and especially if there's a default set that is appended), there's no way to opt out of the behavior. I'm not sure I have a scenario where this would be a problem in practice, but if one arises, there's no longer a way to explicitly not put cmake on the path, other than controlling the environment in which you invoke cibuildwheel - and if cmake is in the same binary path as cibuildwheel, even that option won't work.
- The less often that users need to explicitly define a safe tool set, the less likely it is that the ecosystem as a whole will be aware the option exists at all.
That said - consider this a "strong opinion, weakly held". If y'all would prefer to go for a list of default tools, I'm happy to oblige. And the "soft failure" approach does have some upsides: it means the ultimate failure mode for iOS builds would be the same as other platforms (i.e., no cmake installed? Error when cmake is invoked).
If we do add a default set, I'd suggest:
- Adding
meson
to the default set as well - Making the explicitly provided list an override list, not an augmented list
- Making it a soft failure if a tool isn't found.
Let me know what you'd like me to do and I'll implement that.
@henryiii I think that without a default, scikit-build-core won't work without configuration...
Sure - but a build also won't work on iOS without a CIBW_TEST_SOURCES
configuration. For that matter, cibuildwheel for any moderately complex project won't work on any platform without at least some configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some good points above, thanks @freakboy3742, though I'm still undecided.
My concern is mostly around people getting stuck. It's surprising to have PATH changed out by cibuildwheel, so when users hit the 'mybuildtool: command not found' error, do they even consider that cibuildwheel might have removed it from path? Or do they end up going down a debugging rabbit hole ("okay, i have to install cmake for some reason") before finding this option.
On that note, if we do go the explicit opt-in route, rather than putting default symlinks at the start of path, we could put some entries at the bottom of PATH instead - these symlinks would run a program to print something helpful and error out:
Your build tried to call $0, but this tool isn't listed in cibuildwheel's
`xbuild-tools` option. cibuildwheel clears the PATH variable to remove tools
which commonly provide macOS rather than iOS build configs.
If you're sure "$0" will provide the right options for cross-compilation, add
"$0" to your `xbuild-tools` config. Alternatively, you can add a dir to PATH
using cibuildwheel's `environment` option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yet another option would be to warn if no xbuild-tools
is set in the configuration (iOS only) ? this would raise awareness at the expense of some extra configuration for users that do not require additional tools in order to silence the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joerick Completely agreed that the PATH clearing is unusual/unexpected behavior, and the UX around this is important to get right.
The idea of having a "dummy" tool is an interesting one - if I'm understanding you correctly, we'd use the "link all declared tools, error if they're missing"; with an additional layer that any "known tool" that isn't explicitly declared would be added as a dummy warning script.
However, I think @mayeut's idea is even better. An iOS configuration must define test-sources
; requiring an explicit xbuild-tools=[]
as well doesn't seem too onerous. That requirement can be documented in the iOS platform guide; and if a user doesn't read the documentation (shocking, I know 🤣), it provides a clear and unambiguous reason to raise an error early in the build process. The messaging in that error can point to documentation that gives the deeper explanation, provides a workaround for the simple case, and details how to handle more complex cases. And those more complex cases are completely unambiguous - there's no need for a list of "blessed" tools, or a need to differentiate "cmake" from "other build tools".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so many tradeoffs! Ideally, I'd rather to avoid the concerns of complicated builds polluting the user journey of simple ones.1 Most/many builds would just set xbuild-tools = []
and move on.
An iOS configuration must define
test-sources
; requiring an explicitxbuild-tools=[]
as well doesn't seem too onerous.
True, but only if test-command
is set. The zero-config option still works - i.e. make a pure setuptools package, run cibuildwheel --platform ios
, it builds you some wheels.
To stick with the current train of thought, we could soften the xbuild-tools=[]
requirement a bit - make the missing setting a warning rather than an error. That would save users a modify/commit/push/CI cycle and they'd eventually get the message and set it.
Lets enumerate the options...
- Set a default like
["cmake", "ninja", "swig", "rustc", "meson"]
.- Pros:
- Just works™ for most users
- Simple to implement
- Cons:
- Project-level subjective decision of what to put in this list.
- There's a UX cliff if your build requires something not in our list - the PATH hiding mechanism was hidden from you.
- Pros:
- Insert warning exes for common tools like cmake, ninja, swig, rustc, meson at the bottom of path
- Pros:
- Avoids the UX cliff of not understanding the PATH change
- Cons:
- Only works if the tool you require is in the list
- It's a bit of a hack - would cause issues when combined with a with a user configuration of
CIBW_ENVIRONMENT: "PATH=$PATH:/mybuildtools"
- Pros:
- Raise a warning if
xbuild-tools
is unset, but continue the build assuming that it's[]
.- Pros:
- Avoids the UX cliff of not understanding the PATH change
- Simple to implement
- Cons:
- Requires eventually setting
xbuild-tools
, even if not using any tools.
- Requires eventually setting
- Pros:
On reflection, I'm happy with (3). The other nice thing about (3), is that the warning can say that if the build succeeds, the right value is []
. But if it fails with somebuildtool: command not found
, that should be listed in xbuild-tools
.
Footnotes
-
My philosophy here is 'make the simple stuff easy, make the complicated stuff possible' and 'minimal configuration'. But it's all tradeoffs :) ↩
@@ -1044,6 +1044,35 @@ Platform-specific environment variables are also available:<br/> | |||
[PEP 517]: https://www.python.org/dev/peps/pep-0517/ | |||
[PEP 518]: https://www.python.org/dev/peps/pep-0517/ | |||
|
|||
### `CIBW_SAFE_TOOLS` {: #safe-tools} | |||
> Binaries on the path that are safe to include in an isolated cross-build environment. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If we do specify a default, also list it here.)
msg = f"Could not find a {tool!r} executable on the path." | ||
raise errors.FatalError(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if we do set a default, this would be:)
msg = f"Could not find a {tool!r} executable on the path." | |
raise errors.FatalError(msg) | |
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this a bit differently.
If it's been requested in the configuration, then fail, it's a default tool then continue. (this would probably mean the default list does not initialize safe-tools but rather extends it).
Co-authored-by: Joe Rickerby <[email protected]>
Sometimes, some tool may require internally other tools (by call it's using equivalent of I think it is worth mentioning this in documentation. Some examples of the troubleshooting section. |
Whatever final API we end up with for |
Following on from the discussion on #2286 - This PR adds a
CIBW_SAFE_TOOLS
/safe-tools
setting that can be used to declare executables that are "safe" in a cross-build environment. The implementation builds on the prototype given by @mayeut in this comment.These tools are linked into a
cibw_safe_tools
directory in the cross-platform venv, and that directory is added to the clean path. This allows the Rust special case to be removed, asrustc
can be explicitly added as a "safe" tool if required.I've only exposed the
CIBW_SAFE_TOOLS_<platform>
override for iOS, on basis that it doesn't do anything anywhere else.The PR adds tests to verify that tools can be made safe, but doesn't actually use those tools, as that would require adding an entire project with a (for example) cmake build dependency to check that it works. It does, however, check that cmake can be made a safe tool (on the basis that the tool exists on almost every CI configuration), and that referencing a non-existent tool will raise an error.
To test this, I've forked @mayeut's
pybase64
project to add an iOS configuration of a cmake-built project; this branch contains the changes needed to build and test iOS wheels usingCIBW_PLATFORM=ios cibuildwheel
.Flagging something that I have seen in testing: For some reason, Homebrew's cmake install doesn't seem to play nice with this patch. If Homebrew's version of
cmake
is the one found on the path, the build returns the following error:However, if you
pip install cmake
into the venv that contains cibuildwheel, it works fine. It also works if you download the official CMake tarball and havePATH=.../CMake.app/Contents/bin
in your environment when you build the app.I don't know enough about CMake to diagnose what is going on here. The implication is that there's a
CMAKE_ROOT
environment variable required that might fix things... but I haven't had any luck finding whatCMAKE_ROOT
should be set to. The suggestion of the CMake docs is that it should be the folder that contains theModules
directory... but that doesn't work for me.