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

fix: add missing config-file option to the action #883

Merged
merged 3 commits into from Oct 23, 2021

Conversation

henryiii
Copy link
Contributor

This is missing, but might be handy if users want to use config files other than pyproject.toml.

@henryiii henryiii closed this Oct 22, 2021
@henryiii henryiii reopened this Oct 22, 2021
action.yml Outdated
config-file:
description: 'File containing the config, defaults to {package}/pyproject.toml'
required: false
default: '{package}/pyproject.toml'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we didn't have to specify the default here too, from a DRY perspective.

ahhhh, I think this default might present an issue, actually. If a config file is specified, then cibuildwheel requires it to exist. If it's not set, then the Options class will use pyproject.toml, if it's present. So this wouldn't work for projects without a pyproject.toml.

Maybe we'd have to use some bash to remove the argument if it's not set in the action? Or we could change Options to interpret config_file="" as 'not set'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, yes, I thought of that before, and then forgot about it.

I was thinking --config-file="" would be an option to turn off config file discovery completely; if you want to ignore the config in pyproject.toml, this would be a handy shortcut, possibly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would then also provide different behavior for this option, too, and we haven't supported empty strings here in the past - I've changed this to config_file="" as 'not set'. Someone who wanted to override the pyproject.toml and didn't want a new toml file can just point this at an empty file, I suppose.

@henryiii henryiii force-pushed the henryiii/feat/config-file-action branch from 220918c to 816be41 Compare October 22, 2021 15:44
@henryiii henryiii force-pushed the henryiii/feat/config-file-action branch from 816be41 to 4b7e0be Compare October 22, 2021 15:50
cibuildwheel/__main__.py Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
Co-authored-by: Joe Rickerby <joerick@mac.com>
@henryiii henryiii added the automerge Tells https://github.com/apps/mergery to squash-merge the PR when the button is green. label Oct 22, 2021
@henryiii henryiii merged commit 12c6436 into pypa:main Oct 23, 2021
@henryiii henryiii deleted the henryiii/feat/config-file-action branch October 23, 2021 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Tells https://github.com/apps/mergery to squash-merge the PR when the button is green.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants