Assert that build will fail if there is no configuration to build

[Czaki]

Current revision of cibuildwheel gave no feedback if there is no configuration to build.
There changed platform selector for manylinux. There will be change of selector for macosx.

Currently you add code which will update manulinux1 to manylinux but it is marked as deprecated so in some moment people managing ci environment should get error instead no wheels.

I also change deprecation warning to be printed on stderr instead of stdout.

[YannickJadoul]

Hmmm, I'm quite neutral on adding this, but why not. I'm not sure cibuildwheel should just end with a Python exception thrown, though. I think it's better to print a nice message ourselves and exit.

Currently you add code which will update manulinux1 to manylinux but it is marked as deprecated so in some moment people managing ci environment should get error instead no wheels.

We are already printing warnings for that, though!

I also change deprecation warning to be printed on stderr instead of stdout.

Why? That's sounds a bit unrelated. At any rate, sys.stderr.write is not correct, as it does not add the newline. print(..., file=sys.stderr) is cleaner, I would argue (cfr. StackOverflow).

[YannickJadoul]

print(..., file=sys.stderr) is cleaner

You might need to from __future__ import print_function, btw, to have this work on 2.7, if I read the StackOverflow correctly.

EDIT: Nevermind, it's already there, apparently!

[Czaki]

Yes. You add this information but in stdout. ciuildwheel produces many lines of stdout. So if I do not know what I'm looking for I do not notice this line.
The good pattern is to put such information in stderr. All information which may need action from user should be in stderr.

I stop writing in python 2.7 some times ago an I forget about from __future__ import print_function
And default python2.7 do not allow for this syntax. I change it to use print function.

Hmmm, I'm quite neutral on adding this, but why not. I'm not sure cibuildwheel should just end with a Python exception thrown, though. I think it's better to print a nice message ourselves and exit.

Usage exit create a problem if someone would like to somehow use cibuildwheel as library. You do not allow him to handle situation. Maybe some try ... catch clause in main function?

[YannickJadoul]

Yes. You add this information but in stdout. ciuildwheel produces many lines of stdout. So if I do not know what I'm looking for I do not notice this line.

How is that different from stderr? It doesn't look different in the terminal, right? But it's true that error messages should go to stderr, I agree. I'm just not sure if deprecation warnings count as errors? After all, the program doesn't error?

I stop writing in python 2.7 some times ago an I forget about from __future__ import print_function

Yeah, sorry, I didn't see it was already at the top of the file to have compatibility between 2 and 3.

Usage exit create a problem if someone would like to somehow use cibuildwheel as library. You do not allow him to handle situation. Maybe some try ... catch clause in main function?

Yes, I agree, even though cibuildwheel is definitely not designed/supposed to be used as a library. But catching the exception and printing its message is +- what I had in mind.

[Czaki]

on linux you can pipie stdout and stderr to different files with > (or 1>) for stdout and 2> for stderr.
You can see example here (my project)
https://dev.azure.com/PartSeg/PartSeg/_build/results?buildId=257&view=logs&s=8bfbeaae-4c8e-5f12-f154-edd305817000
The stdout output is hidden and to see it you need to click on step name and stderr output is visible when open the page.

From my experience all messages which which implies some action (now or future) should be written to stderr. Like warnings during C program compilation. The program may compile but author should fix it to be sure if everything is working ok.

[Czaki]

@YannickJadoul I think more about program termination. cibuildwheel is tool for developer, not ordinary users. Current implementation do not catch most of exception (like fail of subprocess.check_call). So why catching this? Reducing information where problem occurs increase the time to solve the problem. Also non catching exception allow to use debuger which can give developer information about source of problem.

[YannickJadoul]

@YannickJadoul I think more about program termination. cibuildwheel is tool for developer, not ordinary users. Current implementation do not catch most of exception (like fail of subprocess.check_call). So why catching this? Reducing information where problem occurs increase the time to solve the problem. Also non catching exception allow to use debuger which can give developer information about source of problem.

My reason for suggesting that was the followin:
Because this is an error message we generate ourselves, and we also print other error messages that have to do with configuration (for example if the platform cannot be detected, or gets bad options, or ...). check_call failing is really some different kind of error coming from the inside out (in my opinion), while this PR just tries to tell the user something extra, rather than actually failing.

But that's just my intuition for keeping things clean and consistent. In the end, @joerick will have the last word on this, ofc.

[joerick]

Hmm, I'm not 100% sure that this should be an error.

I'm guessing the reason for the PR is that setting CIBW_SKIP can be confusing, and it's easy to make a mistake in that config. But how does making this an error help with that?

Also, there might be reasons a user would like no action. E.g., a user that wants to temporarily skip wheels on Windows, for example, might set CIBW_SKIP=*-win* in their travis config. Or maybe somebody is having a tough day fighting the CI, they might set CIBW_SKIP=*. In both cases, the option is fairly clear that the user would like no builds, and cibuildwheel can happily succeed at doing nothing.

Perhaps we could downgrade this to a warning instead?

[Czaki]

But in this case there is everything configured, whole pipeline pass and in some moment user ask why there is no wheel of some type. It is not good if user report you an error. Easiest option is to add next variable, but maybe. for CIBW_SKIP=* it may be replaced with cibuildwheel || true

[YannickJadoul]

@joerick, @Czaki, FWIW, I don't really mind either way. I can see this is a bit confusing from both perspectives. Either when cibuildwheel has had an update and suddenly doesn't test anything anymore when you're expecting it would, or when you're debugging and a responsible adult and you really don't need cibuildwheel whining you didn't build anything.

At any rate, @joerick, even if you choose not to merge this, the commits where warnings are now written to the stderr might be interesting to retain? (I'm not convinced that needs to be tested, though?)

[joerick]

Perhaps we could fix the pain point another way - if there are identifiers in CIBW_BUILD that match nothing, we could raise an error as a misconfiguration? Would that help your use-case @Czaki ?

Easiest option is to add next variable, but maybe

What do you mean by this @Czaki ?

At any rate, @joerick, even if you choose not to merge this, the commits where warnings are now written to the stderr might be interesting to retain? (I'm not convinced that needs to be tested, though?)

Yes, I'd be open to this.

[Czaki]

@joerick

Easiest option is to add next variable, but maybe
What do you mean by this @Czaki ?

Introduction of environment variable, or additional argument maybe easier than user guess.

Perhaps we could fix the pain point another way - if there are identifiers in CIBW_BUILD that match nothing, we could raise an error as a misconfiguration? Would that help your use-case @Czaki ?

looks ok.

[joerick]

Sorry, this wasn't really what I meant @Czaki ... This will raise an error on windows when run with CIBW_BUILD=*linux*, which isn't right in my mind. I think we should pull in all identifiers in __main__ and raise an error if any of the words in CIBW_BUILD have no effect - they're not matching anything from all of the possible identifiers. Would you be able to rewrite to do that?

Sorry for miscommunication!

[Czaki]

@YannickJadoul But this is the case which I think that job should fail. If you do not need windows wheel, then you do not need to run windows worker.

[YannickJadoul]

@Czaki Not sure why you're tagging me; it's not me you need to convince. But I do think @joerick has a point. There might be shared configurations between Linux, Windows, and/or macOS, and CIBW_SKIP="*win*" míght make perfect sense, especially when trying to debug something, for example, as @joerick said.

[Czaki]

@YannickJadoul I dotn know why I mistake you.
@joerick
My current solution work with CIBW_SKIP="*win*". It not work with CIBW_BUILD=*linux* when running on windows host. This protect from lost of all wheels for given platform (because of mistake of configuration, or changes of platform name) , but still allow to manually disable with skip

[YannickJadoul]

My current solution work with CIBW_SKIP="win". It not work with CIBW_BUILD=linux when running on windows host.

I'm not sure I agree with that; that changes the meaning of CIBW_BUILD and CIBW_SKIP (i.e., things to be built are "matched by CIBW_BUILD" minus "matched by CIBW_SKIP"). I don't really like the fact that this would break the symmetry, then.

[Czaki]

@YannickJadoul but this is current implementation

def __call__(self, build_id):
    def match_any(patterns):
        return any(fnmatch(build_id, pattern) for pattern in patterns)
    return match_any(self.build_patterns) and not match_any(self.skip_patterns)

So cibuildwheel build this which are matching CIBW_BUILD and do not match CIBW_SKIP.

I do not expect case when upgrading of cibuildwheel can break configuration on all systems. But can expect case when it broke one of (manylinux1, macosx_10_6, maxosx_*_intel see. #194 (comment) ) and other case when cibuildwheel do not report error but wheel are not created.

If someone try to debug some part of configuration, then can easy protect build from fail when it is intentional (eg. mark step as possible to fail without breaking whole pipeline). But when do simple maintaining then I think that would like to got back response when something stops working.

[YannickJadoul]

So cibuildwheel build this which are matching CIBW_BUILD and do not match CIBW_SKIP.

That is exactly what I said:

"matched by CIBW_BUILD" minus "matched by CIBW_SKIP"

And it feels weird to me if your PR does something that does not match this (by treating CIBW_BUILD different from CIBW_SKIP).

Also, changes of build identifiers is why we have a deprecation period, and give warnings before immediately breaking.

[Czaki]

This change to lost symmetry is this what I initially understand from @joerick request.

I do not understand why to check it globally. Especially user can build wheel only on linux and macos and there will be some pattern on windows that match his configuration and he will got silent fail.

[Czaki]

@YannickJadoul I rethink and agree. I return to check per system and left to Your decision. From my point of view global chcek is useless. If You (especially @joerick) think that this check will create more problem than solve then it sholud be closed.

The idea of this PR is to gave user information about problem as soon as possible.

[joerick]

I do not understand why to check it globally.

I've always thought of the environment that CIBW_ options form as a kind of settings file - options like CIBW_BUILD and CIBW_SKIP only really make sense to me if they're shared between platforms. That's my preferred way to configure cibuildwheel, and platforms like Azure Pipelines let you do that - have one environment that's shared. I believe Github actions will support the same. Maybe one day I'll get around to adding support to supply these options in pyproject.toml, or setup.cfg, or cibuildwheel.toml... So I think about the options in this way.

What your PR does, which I think is good, is alert a user that they might be misconfiguring the tool. This is great, but as long as we're sure the config is an accident. I don't think an error is appropriate unless we're sure there's been a misconfiguration. Something like CIBW_BUILD=*-mnylinux_x86_64 would be an example in my mind, and a global check would catch this.

But no worries if you're not interested in implementing that! I might get around to it soon.

[Czaki]

but if someone set CIBW_BUILD=cp36*64 on macos build it will match for windows and linux build so there will be no warning and no wheel.

Maybe some verification should be done another way? Something like CIBW_EXPECTED_WHEEL_NUM

[joerick]

Closing because I don't agree with the design - apologies @Czaki !