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

Cmake: popen_safe(..., errors='ignore') #6320

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

scivision
Copy link
Member

@scivision scivision commented Dec 9, 2019

probably something to consider making a default for mesonlib/Popen_safe() in general,
but at least for cmake/executor, using

ret = subprocess.run(..., universal_newlines=False, ...)
stdout = ret.stdout.decode(errors='ignore')

instead of Popen_safe() avoids Meson crash/traceback when a dependency(..., method: 'cmake') search goes wrong, and the compiler or Cmake feed back binary garbage not suitable for .decode() implicit in mesonlib/Popen_safe()

That is, this change allows Meson to simply note the dependency as "not found" instead of crashing Meson.

@scivision scivision requested a review from mensinda December 9, 2019 18:26
@scivision scivision added the dependency:cmake Issues related to `dependency` with the `cmake` method label Dec 9, 2019
@scivision scivision added this to the 0.52.2 milestone Dec 9, 2019
mensinda
mensinda previously approved these changes Dec 9, 2019
@scivision scivision dismissed mensinda’s stale review December 9, 2019 18:54

the PR turned out to be not so simple--needs to be rereviewed after

@scivision scivision added the WIP label Dec 9, 2019
@mensinda
Copy link
Member

mensinda commented Dec 9, 2019

We should probably remove this from 0.52.2, if this get's much more complicated.

Also, I doubt that there will be a 0.52.2 at all with 0.53.0 dev freeze 22/12, release 29/12

@mensinda mensinda removed this from the 0.52.2 milestone Dec 9, 2019
@scivision
Copy link
Member Author

great good to know--this is worth fixing for 0.53.0

@scivision
Copy link
Member Author

scivision commented Dec 9, 2019

Actually I think the fix in cmake/executor.py is to use subprocess.run(, errors='ignore') instead of Popen_safe(), which has undesired fallback behavior not helpful for this application. There are other areas of Meson where I also did not use Popen_safe(), where I was not interested in non-UTF8/non-ASCII or other fancy things, just wanted simple text.

@mensinda
Copy link
Member

mensinda commented Dec 9, 2019

Python 3.5 subprocess.run also does not have the errors key. It was added in 3.6 from what I can tell. I guess this would be another reason for #6297.

@scivision
Copy link
Member Author

scivision commented Dec 9, 2019

Hmm, then maybe like

ret = subprocess.run(..., universal_newlines=False)

stdout = ret.stdout.decode(errors='ignore')

@mensinda
Copy link
Member

mensinda commented Dec 9, 2019

To be honest, I don't see the point in having the if sys.version_info >= (3, 6), since the try-catch block does basically the same thing (unless I missed something).

@scivision
Copy link
Member Author

yes it was needless, it's much simpler as in edited comment

mesonlib.Popen_safe() doesn't work with the case where undecodeable
binary data comes back from CMake or compiler, so we use subprocess.run()
@jpakkane jpakkane merged commit 5da1a6e into mesonbuild:master Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency:cmake Issues related to `dependency` with the `cmake` method
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants