fix: warn on deprecated images being set#2312
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
a4ed9aa to
9fa0075
Compare
joerick
left a comment
There was a problem hiding this comment.
I think we'd want to check that the warning doesn't fire loads of times? I think that build_options() is called rather a lot... at least, once per identifier. Perhaps we'd need an instance variable tracking which deprecated images have already been seen.
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
cibuildwheel/logger.py
Outdated
| print(f"cibuildwheel: {c.bold}note{c.end}: {message}\n", file=sys.stderr) | ||
|
|
||
| def warning(self, message: str) -> None: | ||
| def warning(self, message: str, *, deduplicate: bool = False) -> None: |
There was a problem hiding this comment.
Maybe we could just always do this for warnings?
There was a problem hiding this comment.
Think I'd prefer to keep it explicit, it's a bit surprising at the call site otherwise. Might also make some unit tests buggy as this is shared global state.
There was a problem hiding this comment.
^ actually the global state thing might be a reason to do it on Options rather than here.
There was a problem hiding this comment.
I was originally going to make it global as a ClassVar in options. I guess if it was an instance variable it still wouldn't over trigger? It's pretty common for warning to be only shown once globally (the "default" setting for warnings in Python, for example).
There was a problem hiding this comment.
I guess if it was an instance variable it still wouldn't over trigger?
I don't think so. There's only one instance of Options in a normal running process. The only reason I hesitate to do it globally is that I can imagine a unit test expecting to see a warning but depending on test execution order, it sometimes gets triggered somewhere else and the test becomes flaky/hard to debug.
That said it's a hypothetical issue at this point, we can leave it here if you wish.
cibuildwheel/logger.py
Outdated
| print(f"cibuildwheel: {c.bold}note{c.end}: {message}\n", file=sys.stderr) | ||
|
|
||
| def warning(self, message: str) -> None: | ||
| def warning(self, message: str, *, deduplicate: bool = False) -> None: |
There was a problem hiding this comment.
Think I'd prefer to keep it explicit, it's a bit surprising at the call site otherwise. Might also make some unit tests buggy as this is shared global state.
Co-authored-by: Joe Rickerby <joerick@mac.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
1c3d5c4 to
8a190f4
Compare
joerick
left a comment
There was a problem hiding this comment.
Looks good. Thanks for putting it together. I'll roll this into a 2.x patch release.
* fix: warn on deprecated images being set Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix: handle repeated warning messages Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * Apply suggestions from code review Co-authored-by: Joe Rickerby <joerick@mac.com> * fix: try making the warnings once per Options Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Joe Rickerby <joerick@mac.com>
* fix: warn on deprecated images being set Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * fix: handle repeated warning messages Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> * Apply suggestions from code review Co-authored-by: Joe Rickerby <joerick@mac.com> * fix: try making the warnings once per Options Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Joe Rickerby <joerick@mac.com>
See #2047.