'python2 --version' sends output to stderr, redirect to stdout to cat… #20979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
🤖 Beep Boop! This pull request is making changes to 'recipes/qt//'. 👋 @ericLemanissier @jwillikers @MartinDelille you might be interested. 😉 |
This was referenced Nov 7, 2023
ericLemanissier
approved these changes
Nov 7, 2023
ghost
mentioned this pull request
3 tasks
AbrilRBS
requested changes
Nov 16, 2023
recipes/qt/5.x.x/conanfile.py
Outdated
| @@ -151,7 +151,7 @@ def validate_build(self): | |||
| # In any case, check its actual version for compatibility | |||
| from six import StringIO # Python 2 and 3 compatible | |||
| mybuf = StringIO() | |||
| cmd_v = f"\"{python_exe}\" --version" | |||
| cmd_v = f"\"{python_exe}\" --version 2>&1" | |||
There was a problem hiding this comment.
This won't work on Windows - You might need to use subprocess to execute the command directly and get stderr from there
I'll look into allowing Conan's run method to also output stderr just like what is done with stdout
There was a problem hiding this comment.
Hi Ruben,
Thanks for reviewing.
Before submitting I tested the "python --version 2>&1" command in a Windows command prompt and it worked.
A colleague of mine is currently building qt 5.15.11 on Windows and with the above patched he has no errors yet. Before the patch the build would quit almost immediately because of the broken Python2 version check.
So I am quite confident this solution will work also on Windows.
Of course it would still be nice if stderr could be re-directed as well.
There was a problem hiding this comment.
@wdobbe Please, let's avoid workarounds and follow Ruben's approach, the subprocess is specially designed for this case, it's cross-platform compatibly, has fine-grained control over pipes, is more secure (avoids injections), and much better for error handling in case of an exception.
There was a problem hiding this comment.
Changed as requested and tested with Conan 2.13.0 on Linux x86_64 with:
conan create . --version=5.15.11 -o qt/*:qtwebengine=True -o sqlite3/*:shared=True -o qt/*:shared=True -o qt/*:gui=True -o qt/*:qtdeclarative=True -o qt/*:qtlocation=True -o qt/*:qtwebchannel=True --build=missing
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 9948787qt/5.15.11@#330f62d5265b7b1327724f31250726a7qt/5.15.10@#f6a2e57bc3494e5438de6315c34abc14qt/5.15.8@#ca1a0c5f61503f96e13930ba51388c56qt/5.15.7@#fb7ffdac7d421923eb5190af0e79dbcaqt/5.15.9@#be766e2e7f08aa0e6263160232c555e0 |
ericLemanissier
approved these changes
Nov 18, 2023
conan-center-bot
added
Failed
Missing dependencies
This comment has been minimized.
This comment has been minimized.
|
The missing |
|
@valgur Yes, thanks. I already did a rebase and push to re-trigger a build for PR #21026 (also qt/5.x) but it now results in the following error: Comment from Eric Lemanissier:
|
Looking at #21943 it appears it ran into the same problem and it has been fixed now. @wdobbe, could you try merging master again and see if it fixes this pull request too? |
conan-center-bot
added
Failed
Version conflict
Conan v1 pipeline ✔️All green in build 12 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping See details:Failure in build 13 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Hooks produced the following warnings for commit 7604e81qt/5.15.12@#d1d4e7388f85f0a53a29e0a8f3d9803fqt/5.15.11@#b1c474f0b9f5848a8689973171756c3dqt/5.15.9@#8114200f11a945bd061ea454e82dcc80qt/5.15.10@#d9251f76d0eec614394f34ac99beaf53qt/5.15.8@#08176cc0937bb0bf6e7b716ce7d680ebqt/5.15.7@#0c5f73dd2635b31dc7a00463dd16e23b |
valgur
reviewed
Feb 28, 2024
| if completed.returncode != 0: | ||
| msg = ("Found python2 executable %s but could not run it" % python_exe) | ||
| raise ConanInvalidConfiguration(msg) | ||
| verstr = completed.stderr.decode('utf-8').split("Python ")[1] |
There was a problem hiding this comment.
Suggested change
| verstr = completed.stderr.decode('utf-8').split("Python ")[1] | |
| verstr = completed.stderr.decode('utf-8').split("Python ")[1].strip() |
Just to be safe.
valgur
approved these changes
Feb 28, 2024
ghost
mentioned this pull request
3 tasks
|
There is another important point about this issue, it targets Python 2, which is EOL, and Conan 2.x. The Conan 2.x explicitly requires Python 3.6: https://github.com/conan-io/tribe/blob/main/design/003-codebase-python.md |
|
sure, but in qt5 qtwebengine requires python 2, because the chromium version used under the hood requires python 2. |
|
Conan still uses Python 3.x. The python 2 detection is used only for the build of QtWebEngine. As Eric mentioned QtWebEngine 5 requires python 2.On 4 Mar 2024, at 09:13, Uilian Ries ***@***.***> wrote:
There is another important point about this issue, it targets Python 2, which is EOL, and Conan 2.x. The Conan 2.x explicitly requires Python 3.6: https://github.com/conan-io/tribe/blob/main/design/003-codebase-python.md
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This was referenced Mar 18, 2024
ghost
mentioned this pull request
This was referenced Apr 13, 2024
This was referenced May 5, 2024
|
Hi @wdobbe - thank you for your contribution and please accept my apologies for the delay in ensuring this PR went through CI validation. As part of reviewing a different PR, I've proposed a simpler approach that allows us to still call I'll make sure this is reviewed and merged this week. |
|
Closing via #24768. Apologies it took this long to fix this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Specify library name and version: qt/5.15.11
The qt5 qtwebengine module still needs python2 to build it. The qt5 conanfile.py checks if a valid python2 version is installed by running 'python2 --version'.
However the python2 executable sends the version output to stderr (python3 sends it to stdout). As a result the stdout output of the self.run(...) command is empty and results in an error:
Because self.run(...) doesn't seem to have an argument to catch the stderr output I changed the 'python2 --version' command to send stderr output to stdout.
Tested on Linux with conan 2.0.13.