-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat: configuration support #684
Conversation
b29b1c9
to
a686a9f
Compare
@pypa/cibuildwheel-team Before I start working on docs, maybe would be a good idea to look this over? |
Also, not sure why @mayeut is not listed in the above team? @mayeut are you on discord? There's a #cibuildwheel channel there. Edit: If I click on it it lists all four of us. If I hover over, it only lists three, so I guess it's fine. |
I connected once when I saw the thread on https://discuss.python.org. I've just spin it up again, seems there's quite some activity there, I'll try to connect more often. |
@henryiii, |
Looks good overall but I do have 1 major concern for now: a definition in the Minor concern: I find the Here are some tests I added to check how things behaved, the EDIT: tests have been pushed to the PR, removing them from the comment. |
cibuildwheel/options.py
Outdated
if key not in old_dict: | ||
old_dict[key] = {} |
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.
Should we assert key is in old_dict instead (when updating) ?
The key should always be defined in default.toml
no ?
Edit: added when updating
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.
This might not be needed with the check done a bit earlier but, better safe than sorry ?
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 think this is covered, but we need to add some "this should throw an error" type tests to be sure.
Ideally, if we didn't match the environment variable exactly (I plan to list the configuration option along with the environment variable in the help), here are some ideas: # Current (exact match):
[tool.cibuildwheel.manylinux]
x86_64-image = "manylinux2010"
i686-image = "manylinux2010"
# Idea 1: avoid repeating "image":
[tool.cibuildwheel.linux.image]
x86_64 = "manylinux2010"
i686 = "manylinux2010"
# Idea 2: Inside linux directly:
[tool.cibuildwheel.linux]
x86_64-image = "manylinux2010"
i686-image = "manylinux2010"
# Options are in the same "level"
repair-wheel-command = "auditwheel repair -w {dest_dir} {wheel}" Thoughts, @pypa/cibuildwheel-team ? I do think I might agree with @mayeut and prefer Idea 2, but matching the current structure exactly is fine too. |
Actually, thinking about musllinux, this might be best: [tool.cibuildwheel]
manylinux.x86_64-image = "manylinux2010"
manylinux.i686-image = "manylinux2010" That leaves room for musllinux with overlapping archs. Edit: toml supports dotted keys, so the above is actually identical to what we have now. :) This whole thing could go into Linux, though: [tool.cibuildwheel.linux]
manylinux.x86_64-image = "manylinux2010"
manylinux.i686-image = "manylinux2010"
# identical
[tool.cibuildwheel.linux.manylinux]
x86_64-image = "manylinux2010"
i686-image = "manylinux2010"
# identical
[tool.cibuildwheel]
linux.manylinux.x86_64-image = "manylinux2010"
linux.manylinux.i686-image = "manylinux2010" I like it better by itself, I think? |
Also, is "global" best, or would "all" be better? |
What if we just allowed every option to be platform specific, including build, skip, test-skip, and output-dir? Then we could just drop "all/global" and |
Another thought: environment should be a toml table, and extras could be a toml array. |
Sorry for lack of comms on this, I won't be able to give this proper time for the next few days - apologies! |
@henryiii, I like you're proposal for
I think not having "global" or "all" would be better: it seems implied that options in
|
No problem @joerick , it just means you'll get a more polished version to review when you do! :) (It's not going to be ready in the next day or two!)
It wouldn't have to, we could leave
Probably could just be a static check to disallow setting these? Honestly, if someone did set
Yes (I'm not sure about cl args vs env var, but I think I do whatever we currently do there). |
8cabb14
to
706ba88
Compare
The table/list implementation is quite simple. :) For each option, you just describe how to merge a list into a string (and tables are slightly hard coded when formatting, but I think that's fine, as they are only for environments). As a result, you can make archs or build a list, and it works. We could possibly even expand this to make nice lists for commands, using [tool.cibuildwheel]
macos.repair-wheel-command = [
"delocate-listdeps {wheel}",
"delocate-wheel --require-archs {delocate_archs} -w {dest_dir} {wheel}"
] Edit: looks like this works in Windows too, so adding it. Note that out-of-line tables are also valid, it's just TOML: [tool.cibuildwheel.linux.environment]
ENV = "some"
VAR = "setting" It should be noted these do not merge with the global setting (though I could have done that). |
f8003be
to
f6750b8
Compare
Docs at https://cibuildwheel--684.org.readthedocs.build/en/684/options/. @joerick can we set up synced tabs? Like seen on https://packaging.python.org/tutorials/packaging-projects/#uploading-the-distribution-archives , where if you select one item, all matching tabs also change. |
Remaining items:
I see a few tiny docs typos, too. |
eb268e4
to
7f3479b
Compare
1.x: CIBW_BUILD='cp39-*' \
CIBW_MANYLINUX_X86_64_IMAGE=dockcross/manylinux1-x64 \
CIBW_MANYLINUX_I686_IMAGE=dockcross/manylinux1-x86 \
CIBW_BEFORE_BUILD="pip install -r requirements-repair.txt" \
CIBW_REPAIR_WHEEL_COMMAND="python scripts/repair_wheel.py -w {dest_dir} {wheel}" \
CIBW_TEST_REQUIRES="-r requirements-test.txt" \
CIBW_TEST_COMMAND="pytest {project}/tests" \
CIBW_ENVIRONMENT_LINUX="SKBUILD_CONFIGURE_OPTIONS='-DOPENSSL_ROOT_DIR:PATH=/usr/local/ssl -DCMAKE_JOB_POOL_COMPILE:STRING=compile -DCMAKE_JOB_POOL_LINK:STRING=link -DCMAKE_JOB_POOLS:STRING=compile=2;link=1'" \
pipx run cibuildwheel --platform linux Current PR: [tool.cibuildwheel]
build = "cp39-*"
manylinux.x86_64-image = "dockcross/manylinux1-x64"
manylinux.i686-image = "dockcross/manylinux1-x86"
before-build = "pip install -r requirements-repair.txt"
repair-wheel-command = "python scripts/repair_wheel.py -w {dest_dir} {wheel}"
test-requires = "-r requirements-test.txt"
test-command = "pytest {project}/tests"
[tool.cibuildwheel.linux.environment]
SKBUILD_CONFIGURE_OPTIONS = "-DOPENSSL_ROOT_DIR:PATH=/usr/local/ssl -DCMAKE_JOB_POOL_COMPILE:STRING=compile -DCMAKE_JOB_POOL_LINK:STRING=link -DCMAKE_JOB_POOLS:STRING=compile=2;link=1" pipx run cibuildwheel --platform linux |
I'll try to look at this a bit today. At least, I'll try to get some more tests in there. |
I haven't gotten into the implementation yet, just the design and the documentation, as I think these are the most important parts to get right, as the implementation can flow from decisions made at that level. The biggest challenge for this is going to be the documentation. We don't want the addition of the pyproject.toml to make the docs any more confusing, either for new users or existing users of environment variables. In general, I think this is looking pretty good, design-wise. Putting the global/all-platform options under Here are some things on my mind, having thought about this for a couple hours...
Hope this is helpful. Sorry I haven't had more time to dig into this properly! |
Yes, would (slightly reluctantly) be okay with that. When we add musllinux, that would be a different "section", even though it's really very similar. Though it also would be nice to group if there were different arches perhaps. I like "." better than "-" if something is really "part of" something - the different images are logically part of manylinux, and pro TOML user can even take advantage of it. Note that they can always be placed in "linux" as well, we could always document them that way.
Multiline commands are currently rather irritating to work with, requiring manual chaining - I thought this seemed like a good way to write them - and it has a strong precedent in YAML, where Travis and others allows a list of commands. I intentionally showed different ways to write things in the examples, rather than keeping a consistent "recommended" style. Though we can converge on one. One key "quirk" of the pyproject.toml examples is that they have a single header at the top, but then show lots of examples of a value being set without putting a header on each one. It makes it look a bit like it's a complete file, rather than multiple examples. (Guessing at what might be hard to put your finger on) |
5fd51d0
to
84e33a3
Compare
Closes #547.
Design:
tool.cibuildwheel
holds options with a structure that exactly matches the environment variables, just with dashes used instead of_
to match TOML style.tool.cibuildwheel.<platform>
holds the platform specific commands; these are identical but override the global value if defined and running on that platform. The defaults are defined incibuildwheel/resources/defaults.toml
. Environment variables override the pyproject.toml values, which override the defaults.toml values. An error is thrown if an unexpected option is found insidetool.cibuildwheel
or a section based on what was found in the defaults.toml file.CIBW_PROJECT_REQUIRES_PYTHON
does not participate, since it has a canonical location in the pyproject.toml file already, andCIBW_PLATFORM
really is only intended to be non-static, so it also does not participate. I have also removedCIBW_OUTPUT_DIR
; having that "hidden" in a config file might be confusing? It's already a command line option, an action option, and an environment variable. Also this avoids having a way to specify it per platform.I have not added a check to see if
manylinux-*
is in windows/macOS sections (it would do nothing if placed there), ordependency-versions
in linux. Those could be added later.Tables and arrays automatically stringify using
sep
, so those are recommended but not required for use for things like environment and extras. Strings always behave exactly the same as in the environment variables.defaults.toml
, which serves as the canonical source for defaults, and an example: