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

feat: configuration support #684

Merged
merged 29 commits into from
Jun 21, 2021
Merged

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented May 22, 2021

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 in cibuildwheel/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 inside tool.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, and CIBW_PLATFORM really is only intended to be non-static, so it also does not participate. I have also removed CIBW_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), or dependency-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:

[tool.cibuildwheel]
build = "*"
skip = ""
test-skip = ""

archs = ["auto"]
dependency-versions = "pinned"
environment = {}
build-verbosity = ""

before-all = ""
before-build = ""
repair-wheel-command = ""

test-command = ""
before-test = ""
test-requires = []
test-extras = []

manylinux-x86_64-image = "manylinux2010"
manylinux-i686-image = "manylinux2010"
manylinux-aarch64-image = "manylinux2014"
manylinux-ppc64le-image = "manylinux2014"
manylinux-s390x-image = "manylinux2014"
manylinux-pypy_x86_64-image = "manylinux2010"
manylinux-pypy_i686-image = "manylinux2010"
manylinux-pypy_aarch64-image = "manylinux2014"


[tool.cibuildwheel.linux]
repair-wheel-command = "auditwheel repair -w {dest_dir} {wheel}"

[tool.cibuildwheel.macos]
repair-wheel-command = [
  "delocate-listdeps {wheel}",
  "delocate-wheel --require-archs {delocate_archs} -w {dest_dir} {wheel}",
]

[tool.cibuildwheel.windows]

@henryiii
Copy link
Contributor Author

@pypa/cibuildwheel-team Before I start working on docs, maybe would be a good idea to look this over?

@henryiii
Copy link
Contributor Author

henryiii commented May 24, 2021

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.

@mayeut
Copy link
Member

mayeut commented May 25, 2021

@mayeut are you on discord?

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.

@mayeut
Copy link
Member

mayeut commented May 25, 2021

@henryiii,
Thanks for drafting this. I'm still playing a bit with this, I will follow up here with remarks shortly.

@mayeut
Copy link
Member

mayeut commented May 25, 2021

Looks good overall but I do have 1 major concern for now: a definition in the tool.cibuildwheel.global section of pyproject.toml does not override the definition in tool.cibuildwheel.{platform} section of default.toml (repair-wheel-command is the only one for now).

Minor concern: I find the tool.cibuildwheel.manylinux section odd given a tool.cibuildwheel.linux section exists. I understand it would not be 1<->1 with how env vars are working now but looking at a configuration file, I'd expect to find them in tool.cibuildwheel.linux section.

Here are some tests I added to check how things behaved, the test_project_global_override_default_platform is not passing (my major concern), I'll check if this can be fixed quickly and add a commit with those tests if successful.

EDIT: tests have been pushed to the PR, removing them from the comment.

Comment on lines 95 to 93
if key not in old_dict:
old_dict[key] = {}
Copy link
Member

@mayeut mayeut May 25, 2021

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

Copy link
Member

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 ?

Copy link
Contributor Author

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.

@henryiii henryiii mentioned this pull request May 25, 2021
12 tasks
@henryiii
Copy link
Contributor Author

henryiii commented May 25, 2021

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.

@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

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?

@henryiii
Copy link
Contributor Author

Also, is "global" best, or would "all" be better?

@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

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 tool.cibuildwheel.<plat>.X would always override tool.cibuildwheel.X for all X. It might be a little silly to customize the output directory per OS, but it would be a simple and consistent rule with no surprises. (And we could also not allow output dir to be in the toml)

@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

Another thought: environment should be a toml table, and extras could be a toml array.

@joerick
Copy link
Contributor

joerick commented May 27, 2021

Sorry for lack of comms on this, I won't be able to give this proper time for the next few days - apologies!

@mayeut
Copy link
Member

mayeut commented May 27, 2021

This whole thing could go into Linux, though

@henryiii, I like you're proposal for manylinux. I was going to bring up musllinux but you beat me to it.

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 tool.cibuildwheel..X would always override tool.cibuildwheel.X for all X. It might be a little silly to customize the output directory per OS, but it would be a simple and consistent rule with no surprises. (And we could also not allow output dir to be in the toml)

I think not having "global" or "all" would be better: it seems implied that options in tool.cibuildwheel would be global when reading a configuration file.
Having said that, your comment about allowing every option to be platform specific raises a few questions:

  • would it change the current environment variables (probably not, it would just extend the ones already supported ?)
  • how to disallow specifying a manylinux image on windows/macOS (i.e. have some options really specific to a platform) ?
  • I've not checked the relation with command line args yet, I would assume command line arg overrides env var overrides config file ?

@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

Sorry for lack of comms on this, I won't be able to give this proper time for the next few days - apologies!

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!)

would it change the current environment variables (probably not, it would just extend the ones already supported ?)

It wouldn't have to, we could leave CIBW_SKIP_LINUX from working if we want; but tool.cibuildwheel.linux.skip would work. It is much easier to explain if anything can be placed in "<plat>", it a bit more useful for static configs, and while people usually look up env vars that are available, for static configs, they usually go from an example.

how to disallow specifying a manylinux image on windows/macOS (i.e. have some options really specific to a platform) ?

Probably could just be a static check to disallow setting these? Honestly, if someone did set tool.cilbuildwheel.windows.manylinux.x86_64-image, I would also be okay with just considering it unused and ignoring it. 🤦

I've not checked the relation with command line args yet, I would assume command line arg overrides env var overrides config file ?

Yes (I'm not sure about cl args vs env var, but I think I do whatever we currently do there).

@henryiii henryiii force-pushed the henryiii/feat/config branch 2 times, most recently from 8cabb14 to 706ba88 Compare May 27, 2021 14:27
@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

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 sep="; " or sep=" && ", perhaps, depending on the OS? For example, this would work with sep=" && " for macOS & linux:

[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).

@henryiii
Copy link
Contributor Author

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.

@henryiii
Copy link
Contributor Author

henryiii commented May 27, 2021

Remaining items:

  • Probably should have more tests. Happy to accept help with making good tests. ;P
  • Should we explicitly disallow joining arrays for fields that don't need it? All fields can be written as an array currently, and will be joined with a space if not requested otherwise. It could be handy in some of the cases for readability, like build = ["cp*", "pp*"]; you could have one per line, etc.
  • Should we disallow "bad" platform settings? manylinux in windows/macOS, dependency-versions in linux?
  • Is it best to keep tool.cibuildwheel.output-dir out? I don't think it's even in our help file already in environment form.
  • Maybe we could put a mention of the config in a few more places.
  • Should I allow CIBW_BUILD_<plat>and such for the three variables? (BUILD, SKIP, TEST_SKIP)? While not technically needed, it would simplify the logic a little and be a bit more consistent. I'm fine with it the current way, though, where they don't have platform envvar versions.

I see a few tiny docs typos, too.

@henryiii
Copy link
Contributor Author

henryiii commented May 29, 2021

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

@mayeut
Copy link
Member

mayeut commented May 29, 2021

I'll try to look at this a bit today. At least, I'll try to get some more tests in there.

@joerick
Copy link
Contributor

joerick commented May 30, 2021

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 [tool.cibuildwheel] and platform ones under [tool.cibuildwheel.linux] makes lots of sense, and is what I'd expect, I think.

Here are some things on my mind, having thought about this for a couple hours...

  • (design) The manylinux images option... I think either we should leave it more-or-less as-is (just strings called manylinux-x86_64-image = "manylinux2010") or change it completely, to be manylinux-image = { x86_64: "manylinux2010", ... }. I'd lean toward the former, just leaving as a collection of simple string options, because I think it's easier to understand the duality between the env vars and the pyproject.toml options, especially in the documentation. ✅
  • (docs) Putting manylinux images into their own section of the pyproject.toml is also a little weird, as they only affect the linux build, so they should be either globally scoped, or within [tool.cibuildwheel.linux] - probably both should work. Ah, I see that this isn't a section, it's a toml dict. Hmm. Even though I understand how TOML works, this is very confusing for me, because manylinux looks like a sibling of linux when written like that i.e. a different platform. I think we should avoid using any TOML headers other than [tool.cibuildwheel] and [tool.cibuildwheel.platform] in the documentation. Pro TOML users would still be able to use these other dict forms, but most just want to write ini-style configs I think. ✅
  • (docs) on that note, I noticed that the archs docs is using another TOML feature that confused me a little - macos.archs: ["x86_64", "universal2", "arm64"]. The dotted keypath is neat, but I don't think it belongs in reference documentation. The name of the option is archs, so we should have examples that say archs = .... Nesting them within a [tool.cibuildwheel.<platform>] will help reinforce that concept for users, too. ✅
  • (design) I like the list variant of some of the options - archs is a good example. environment also works great as an inline dict. Maybe we should also include test_requires in that. But I'm less sure about the commands being lists, like repair_command. This was surprising, because we don't think about 'commands' being 'lists'. And I don't think it's worth the extra headscratching when users first come across it. changed my mind on this - it's currently the only cross-platform way to do multiline commands, so we're keeping it! ✅
  • (docs) on the topic of the list-style configs, lots of options are already documented as 'a space-separated list of…'. Maybe that should be changed to 'a list of…' and we can lean on the examples to show how that differs between environment variables and pyproject.toml. ✅
  • (docs) I think it's probably the right approach to keep env var as primary (in the titles and the nav), as you have done. ✅
  • (docs) I can't exactly put my finger on it, but the examples aren't as easy to read in this new form. Maybe it's a visual design thing. One minor thing that might improve this would be to restore the "Examples" header and then make the tabs called "Environment variables" and "pyproject.toml". ✅

Hope this is helpful. Sorry I haven't had more time to dig into this properly!

@henryiii
Copy link
Contributor Author

henryiii commented May 30, 2021

The manylinux images option

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.

But I'm less sure about the commands being lists

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)

@henryiii henryiii merged commit f62cbc3 into pypa:main Jun 21, 2021
@henryiii henryiii deleted the henryiii/feat/config branch June 21, 2021 16:26
@joerick
Copy link
Contributor

joerick commented Jun 21, 2021

🎉 woot! great work @henryiii and @mayeut !

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.

3 participants