modules/python: detect debian distutils missing and show an error #9335
Conversation
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
27130d0 to
7cc10b4
Compare
Codecov Report
@@ Coverage Diff @@
## master #9335 +/- ##
==========================================
- Coverage 67.29% 67.29% -0.01%
==========================================
Files 396 396
Lines 85410 85420 +10
Branches 17481 17485 +4
==========================================
+ Hits 57474 57480 +6
- Misses 23246 23248 +2
- Partials 4690 4692 +2
Continue to review full report at Codecov.
|
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
7cc10b4 to
9fff908
Compare
There was a problem hiding this comment.
In general I like the look of this. But I'm not sure we should be logging an error directly in the Python ExternalProgram constructor, since it may be requested with required: false and hence not be an error after all.
It should probably either:
- be logged as mlog.log and rely on the later checks for sanity() return value to promote to an error
- set an attribute on self and look that up later to raise the error message as mlog.error
mesonbuild/modules/python.py
Outdated
| if info is not None: | ||
| if 'error' in info: | ||
| assert isinstance(info['error'], str) | ||
| mlog.error('Python interpreter introspection failed: %s' % info['error']) |
There was a problem hiding this comment.
Please use f-strings (or, when the complexity penalty is too great, the str.format() method).
The assert doesn't seem to be performing any debug sanity checks, so it can probably be dropped.
There was a problem hiding this comment.
The assert is to make sure the nobody screw up when editing the introspection script, but I can remove it if you want.
There was a problem hiding this comment.
Actually that may speak to a greater need: adding a test case that actually tries to trigger this code path and testing that it produces the correct error message.
On the other hand -- if it's not a string what will it be? It is loaded as json there are only so many options...
There was a problem hiding this comment.
I don't have strong opinions on the assert, but as a project standard we have purged percent formatting in favor of f-strings wherever it doesn't have major readability costs (now that we require a minimum python which supports f-strings) and the .format() method otherwise.
There was a problem hiding this comment.
Actually that may speak to a greater need: adding a test case that actually tries to trigger this code path and testing that it produces the correct error message.
We would have to keep the tests updated to make sure they cover all code paths in the introspection command, which kind of defeats the purpose. I have already added a test that triggers the code path from the Meson side, just not the introspection script.
On the other hand -- if it's not a string what will it be? It is loaded as json there are only so many options...
Could be anything. I just put the assert there as a sanity check to avoid obscure error. Yes, this is me being preventive, but it's cheap, so...
I don't have strong opinions on the assert, but as a project standard we have purged percent formatting in favor of f-strings wherever it doesn't have major readability costs (now that we require a minimum python which supports f-strings) and the .format() method otherwise.
Of course. I don't like nesting quotes in f-strings, so I went with % as that's what the neighboring code did, but I can definitely change.
There was a problem hiding this comment.
Nesting quotes? Do you mean like f'{some string!r}' to guarantee that the printed content itself has quote characters?
Edit: oh sorry, I'm dumb. You meant the single quotes for the dictionary key. Yeah, that can be a bit annoying.
There was a problem hiding this comment.
f'something: {info["error"]}'There was a problem hiding this comment.
Also it is true we do have some percent formatted strings still... I would like to get rid of all of them but I mainly drove the upgrade using pyupgrade which is pretty good but still heuristics based and thus not perfect. The vast majority were changed though. :D
There was a problem hiding this comment.
Anyway, I changed to str.format, let me know if you'd like me to change to an f-string.
I would prefer this. If my Python interpreter is bad and consequently the Python specific bits of the project gets disabled, I want to know why. |
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
9fff908 to
539f3a6
Compare
|
That's a reasonable perspective and I think I agree. |
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
539f3a6 to
df77112
Compare
|
Python 3.10 (scheduled to be released on Monday) deprecates ditsutils and Python 3.12 drops support for distutils as discussed in https://www.python.org/dev/peps/pep-0632/ |
|
Yes, we are aware, but unfortunately we don't have a migration plan right now due to Debian patching the interpreter, but not sysconfig. As distutils will only be removed in 3.12 we still have some time to coordinate and update the code. |
|
The deprecation of distutils is unrelated to this PR as we already depended on distutils for entirely unrelated reasons. For a long time, meson has considered the lack of distutils to be a fatal error. This PR merely changes the formatting of the error message that is used when meson fatally exits. Please take all discussion of distutils deprecation to #7702. And distutils is not deprecated right now! It is only deprecated in python 3.10! This is important, because it means we have an awful long time before it is actually removed, which means that Debian's failure to have a legitimate python is not a valid reason for us to make meson not work on Debian in order to punish ordinary Debian users for the fact that Debian developers do not comply with the PEP. It's not a bug to rely on deprecated things. It is a concern, yes, but that just means we need to balance the pros and cons of need vs. consequences. |
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
df77112 to
9f5cb9d
Compare
|
Let's see if restarting the CI job fixes that macOS failure. It looks pretty irrelevant anyway... |
| def test_introspection_error(self, mlog_log, Popen_safe): | ||
| python = PythonExternalProgram('some-python', ['some-python']) | ||
|
|
||
| assert not python.sanity() |
There was a problem hiding this comment.
Hmm, shouldn't this be a unittest assertFalse, not a regular assert?
There was a problem hiding this comment.
When tests are run with pytest, either is acceptable. In fact, this simplicity without loss of debugging information is pytest's number one feature.
There was a problem hiding this comment.
Well yes, but if you don't have both pytest and pytest-xdist then the tests get run without pytest, just unittest, and that is a supported configuration...
There was a problem hiding this comment.
It's the same thing in this case, these functions just exist to provide a nicer error output. pytest rewrites the bytecode to replace the asserts with something similar in order to achieve that. It makes sense when you are comparing lists for eg, but as we are just checking a bool, is pretty much the same.
There was a problem hiding this comment.
I know what pytest does, thanks.
Again, my point is that we don't depend on pytest (even if we do automatically use it some of the time).
There was a problem hiding this comment.
If it's "pretty much the same thing" to you, it shouldn't be a problem to use unittest asserts for the sake of those odd people using unittest instead of pytest to run this testsuite :)
There was a problem hiding this comment.
It's pretty much the same thing because we are just checking a bool, which does not show any extra information, unlike comparing lists for eg.
I just used an assert because that's what I am used to, but I can change it if you want. Though I am not home right now, and have an appointment afterwards, so it might be a couple hours.
|
Due to the previous PR which I just merged, this now ⚔️ conflicts. Other than that and the assert nit, I approve this PR. |
|
⚔️ conflicts. |
|
Urgent ping. 0.60 rc1 is due today and we're trying to merge things we think are ready ASAP beforehand. I would love to see this make it in! |
|
I will update it tomorrow when I am on working hours, unless it cannot wait until then. I will also try to send a patch for ev-br/mc_lib#45 ASAP. |
|
Seems like the rc is being delayed until tomorrow anyway due to time constraints. :) Looking forward to the update for this PR which should still make it in. |
Signed-off-by: Filipe Laíns <[email protected]>
FFY00
force-pushed
the
better-debian-distutils-error
branch
from
9f5cb9d to
2c368b4
Compare
|
@eli-schwartz rebased on |
|
What happened? |
Signed-off-by: Filipe Laíns [email protected]