SCons: Decode stdout from subprocess #2201

uklotzde · 2019-07-08T18:43:13Z

Try to fix some issues with SCons 3 reported here: #1356 (comment)

Holzhaus · 2019-07-08T19:27:55Z

build/mixxx.py

@@ -213,15 +213,15 @@ def __init__(self, target, machine, build, toolchain, available_features):
        # Now that environment variables have been read, we can detect the compiler.
        import subprocess
        process = subprocess.Popen("%s %s" %(self.env['CC'], '--version'), stdout=subprocess.PIPE, shell=True) # nosec
-        (stdout, stderr) = process.communicate()
+        stdout = process.communicate()[0].rstrip().decode()


Might make sense to use subprocess.check_output() instead.

Thanks for the hint!

I just read that process.run() is recommended instead of subprocess.check_output() since 3.5? I'm not able to decide what to choose here, considering that we need to support a variety of Python versions. Maybe someone else can take over and fix all those issues?

Since you only want to check the content of stdout anyway, there's no actual advantage from using subprocess.run() other than being hip and modern. On the other hand, we'd lose compatibility with all Python versions < 3.5 (subprocess.check_output even works on Python 2.7).

Holzhaus · 2019-07-08T19:38:05Z

build/mixxx.py

@@ -213,15 +213,15 @@ def __init__(self, target, machine, build, toolchain, available_features):
        # Now that environment variables have been read, we can detect the compiler.
        import subprocess
        process = subprocess.Popen("%s %s" %(self.env['CC'], '--version'), stdout=subprocess.PIPE, shell=True) # nosec


shell=True in combination with user input opens the door for command injections, e.g. if CC is set to rm -rf /; gcc.
Instead, something like this should be used:

process = subprocess.Popen([self.env['CC'], '--version'], stdout=subprocess.PIPE)` # or: stdout = subprocess.check_output([self.env['CC'], '--version'])`

The shell=True argument has been added just recently by the NixOS build support, not sure why.

@poelzi has explained it here:
https://github.com/mixxxdj/mixxx/pull/2144/files#r291847013
Unfortunately the comment was not added to the code. Maybe we can add a comment here?

In that case IMHO it's still undesirable to invoke a shell because we don't know what crazy non-standard $SHELL the user is using. shlex.split() can be used to split the content of CC env var, so that shell=True is not needed.

stdout = subprocess.check_output([*shlex.split(self.env['CC']), '--version'])` # or the longer version cmd = shlex.split(self.env['CC']) cmd.append('--version') stdout = subprocess.check_output(cmd)`

uklotzde · 2019-07-14T11:05:12Z

@Holzhaus

Is .decode(sys.stdout.encoding) required?
Replacing Popen() with check_output() in depends.py: find_framework_libdir fails with "coercing to Unicode: need string or buffer, NoneType found"

Holzhaus · 2019-07-20T07:53:41Z

@uklotzde Sorry for the late answer, I'm currently traveling. Since subprocess.check_output() returns bytes (not str), it is necessary to decode the return value. However, .decode() without an explicit encoding should suffice.

I couldn't reproduce the issue from your second point, but have a look at this example: uklotzde/mixxx@scons...Holzhaus:scons
If you need to retain python 2.7 compatibility, use OSError instead of FileNotFoundError.

rrrapha · 2019-07-20T09:42:03Z

Note that this changes the behaviour of the compiler check. It fails if a compiler doesn't support --version.
For example, the following throws an error now (and did not before):

$ export CC=
$ scons

Not sure if it's a real problem..

daschuer · 2019-07-21T22:46:02Z

I can confirm that this still builds on my Ubuntu Xenial using Scons v2.4.1.

uklotzde · 2019-07-22T05:42:58Z

I'm not experienced enough with Python and will not change this code again. Please take over if you know a better way.

Imports that are not at the top of file are considered bad style in Python. See this for details: https://legacy.python.org/dev/peps/pep-0008/#imports

Holzhaus · 2019-08-11T16:08:50Z

@uklotzde I made some smaller fixes and improvements:
uklotzde/mixxx@scons...Holzhaus:scons

Build fine for me on Linux now (I had some problems with the pkg-config invocation so that Qt couldn't be found.)

uklotzde · 2019-08-11T17:28:29Z

The build with Scons 3.0.4 / Python 2.7 fails now:

python2-scons

TypeError: glob() got an unexpected keyword argument 'recursive':
  File "/run/media/uk/Private/tmp/mixxx/SConstruct", line 82:
    SConscript('SConscript.env', variant_dir=build.build_dir, duplicate=0)
  File "/usr/lib/python2.7/site-packages/SCons/Script/SConscript.py", line 667:
    return method(*args, **kw)
  File "/usr/lib/python2.7/site-packages/SCons/Script/SConscript.py", line 604:
    return _SConscript(self.fs, *files, **subst_kw)
  File "/usr/lib/python2.7/site-packages/SCons/Script/SConscript.py", line 285:
    call_stack[-1].globals)
  File "/run/media/uk/Private/tmp/mixxx/SConscript.env", line 106:
    dep_sources = dependency.sources(build)
  File "/run/media/uk/Private/tmp/mixxx/build/depends.py", line 718:
    sources = glob.glob('src/**/*.cpp', recursive=True)

Unfortunately the build fails due to another reason when using the same SCons version on Python 3.7 as reported on Launchpad:

python3-scons

[MOC] src/controllers/midi/portmidienumerator.h
[CXX] src/controllers/midi/portmidicontroller.cpp
[CXX] lin64_build/src/controllers/midi/moc_portmidienumerator.cc
[CXX] src/controllers/midi/portmidienumerator.cpp
[CXX] src/effects/lv2/lv2backend.cpp
[CXX] src/effects/lv2/lv2effectprocessor.cpp
[CXX] src/effects/lv2/lv2manifest.cpp
[MOC] src/effects/lv2/lv2backend.h
[CXX] src/encoder/encoderopus.cpp
[CXX] lin64_build/src/effects/lv2/moc_lv2backend.cc
[CXX] src/engine/bufferscalers/enginebufferscalerubberband.cpp
[CXX] src/engine/bufferscalers/enginebufferscalest.cpp
[MOC] src/engine/bufferscalers/enginebufferscalerubberband.h
[CXX] lin64_build/src/engine/bufferscalers/moc_enginebufferscalerubberband.cc
[MOC] src/engine/bufferscalers/enginebufferscalest.h
[CXX] lin64_build/src/engine/bufferscalers/moc_enginebufferscalest.cc
[MOC] src/engine/controls/vinylcontrolcontrol.h
[CXX] src/engine/controls/vinylcontrolcontrol.cpp
[CXX] lin64_build/src/engine/controls/moc_vinylcontrolcontrol.cc
[MOC] src/engine/sidechain/shoutconnection.h
[CXX] lin64_build/src/engine/sidechain/moc_shoutconnection.cc
[CXX] src/engine/sidechain/shoutconnection.cpp
[UIC5] src/preferences/dialog/dlgprefbroadcastdlg.ui
[CXX] src/preferences/dialog/dlgprefbroadcast.cpp
[CXX] src/preferences/dialog/dlgpreflv2.cpp
In file included from src/preferences/dialog/dlgpreflv2.cpp:8:
src/preferences/dialog/dlgpreflv2.h:7:10: fatal error: preferences/dialog/ui_dlgpreflv2dlg.h: No such file or directory
    7 | #include "preferences/dialog/ui_dlgpreflv2dlg.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scons: *** [lin64_build/src/preferences/dialog/dlgpreflv2.o] Error 1
scons: building terminated because of errors.

The additional repo_node kwarg is required since this commit: SCons/scons@f999d21#diff-ac9db0da66287699ffbaf33c370d2c85R1508

Holzhaus · 2019-08-11T19:05:15Z

@uklotzde Sorry, I'm currently on my old and slow laptop, so I just Ctrl-C'ed after the configuration stage.

glob.glob only supports recursive mode since Python 3.5. Just revert uklotzde@dfef9c7 for now. This should also fix the other issue for now (but IMHO the root cause is the broken dlgpreflv2.cpp file, which should either be fixed or removed).
I dropped that commit from my branch.
We can add it again in approximately 4 month and 20 days, when Python 2 will finally be retired ;-)

I also had to add another commit to fix compilation with SCons 3.1.1:
uklotzde@d3562f1

I can confirm that compilation works with Scons 3.1.1 and Python 3.7.

Be-ing · 2019-08-11T19:14:47Z

IMHO the root cause is the broken dlgpreflv2.cpp file, which should either be fixed or removed

I intend to eventually remove that as part of #1705, but that PR still needs a lot of work before it is ready for merge so don't count on that happening soon.

Holzhaus · 2019-08-12T22:09:50Z

IMHO the root cause is the broken dlgpreflv2.cpp file, which should either be fixed or removed

I intend to eventually remove that as part of #1705, but that PR still needs a lot of work before it is ready for merge so don't count on that happening soon.

Ok, don't worry. It's not time critical since I remove the commit (that replaced the hardcoded sources with globbing) anyway. If we add it again, we could add a blacklist to exclude files instead, which would still be shorter than the hardcoded whitelist.

@uklotzde Can we merge this or are there issues with older SCons/Python versions? Currently, compilation is completely broken for me without these changes (Scons 3.11, Python 3.7).

This re-adds 'utf-8' to bytes.decode() calls and replaces FileNotFoundError with OSError to restore compatibility with Python 2.7. When Python 2.7 finally reaches EOL we can simply revert this commit.

Holzhaus · 2019-08-23T11:33:32Z

@uklotzde I had a look at the TravisCI build log. Looks like it failed due to missing Python 2.7 compatibilty. Here are some more fixes that restores it: uklotzde/mixxx@scons...Holzhaus:scons

Works with SCons 3.0.4/Python 2.7.16 and SCons 3.1.1/Python 3.7.4 (tested on Linux/AMD64).

Holzhaus · 2019-08-23T11:36:27Z

By the way, let's merge this ASAP. It's getting annoying that I need to apply/stash Scons-3.1-related fixes on every branch that I'm working on ;-)

uklotzde · 2019-08-23T12:17:28Z

Let's wait until the CI build have successfully completed. Since no one else takes care of this rather urgent issues I will merge it asap!! Thanks for providing all these patches.

I will try a clean Python 3 / SCons 3.0.4 build on fc30, although I don't expect it to succeed.

uklotzde · 2019-08-23T12:22:02Z

As expected, a clean build with Python 3 / SCons 3.0.4 still fails:

[CXX] src/analyzer/trackanalysisscheduler.cpp
In file included from src/library/analysisfeature.h:16,
                 from src/library/library.h:18,
                 from src/analyzer/trackanalysisscheduler.cpp:3:
src/library/dlganalysis.h:11:10: fatal error: library/ui_dlganalysis.h: No such file or directory
   11 | #include "library/ui_dlganalysis.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scons: *** [lin64_build/src/analyzer/trackanalysisscheduler.o] Error 1

I delete lin64_build/, .sconf_temp/ and .sconsign.dblite for a clean build.

Holzhaus · 2019-08-23T13:03:31Z

@uklotzde Sorry, I used scons -c to clean up. Looks like that doesn't suffice though. I'm getting the same error as you when using Scons 3.1.1 and Python 3.7.4.

However, this doesn't seem to be related to this PR, since it also happens when I try to build the current master branch with this patch:

$ git stash show -U
diff --git a/build/mixxx.py b/build/mixxx.py
index b9501ee81a..456f079685 100644
--- a/build/mixxx.py
+++ b/build/mixxx.py
@@ -214,8 +214,8 @@ class MixxxBuild(object):
         import subprocess
         process = subprocess.Popen("%s %s" %(self.env['CC'], '--version'), stdout=subprocess.PIPE, shell=True) # nosec
         (stdout, stderr) = process.communicate()
-        self.compiler_is_gcc = 'gcc' in stdout.lower()
-        self.compiler_is_clang = 'clang' in stdout.lower()
+        self.compiler_is_gcc = 'gcc' in stdout.decode().lower()
+        self.compiler_is_clang = 'clang' in stdout.decode().lower()

         # Determine the major compiler version (only GCC)
         if self.compiler_is_gcc:
@@ -225,7 +225,7 @@ class MixxxBuild(object):
             gcc_version = stdout
             # If match is None we don't know the version.
             if not gcc_version is None:
-                version_split = gcc_version.split('.')
+                version_split = gcc_version.decode().split('.')
                 if version_split:
                     self.gcc_major_version = int(version_split[0])

I'll investigate.

uklotzde · 2019-08-23T13:22:57Z

Definitely not caused by your changes. It's a known and long outstanding bug that we need to solve soon as Python 2 becomes deprecated:

https://bugs.launchpad.net/mixxx/+bug/1817742

Not sure if anyone is working on this already? I didn't get any notice from "andy-xyz" after he has assigned the bug to himself.

uklotzde · 2019-08-23T13:26:31Z

Unfortunately macOS/clang build now fails:

Configuring Qt
Package Qt5Core was not found in the pkg-config search path.
Perhaps you should add the directory containing `Qt5Core.pc'
to the PKG_CONFIG_PATH environment variable
No package 'Qt5Core' found
ERROR:root:Unmet dependency: global name 'FileNotFoundError' is not defined

Holzhaus · 2019-08-23T13:40:17Z

@uklotzde

Unfortunately macOS/clang build now fails

Sorry, I accidently grepped just build/mixxx.py instead of the whole build directory.

Definitely not caused by your changes. It's a known and long outstanding bug that we need to solve soon as Python 2 becomes deprecated:

https://bugs.launchpad.net/mixxx/+bug/1817742

Not sure if anyone is working on this already? I didn't get any notice from "andy-xyz" after he has assigned the bug to himself.

I found the issue. Python's map() function is used to invoke uic for each *.ui file. In Python 3, map() returns an iterator which is evaluted on-demand (i.e. the next element is read) rather than when map() is called. This can save memory and computation time, but we're never reading map()'s results, so uic won't be executed at all.

Here are some more patches that should fix the remaining issues:
uklotzde/mixxx@scons...Holzhaus:scons

Works with SCons 3.0.4/Python 2.7.16 and SCons 3.1.1/Python 3.7.4 (and this time i rm -rf'ed lin64_build, .scons_temp and .sconsign.dblite instead of using scons -c :D).

uklotzde · 2019-08-23T14:21:43Z

It just works now!! That was awesome, Jan 👍

I'm always afraid if we need to touch this brittle and hand-crafted build system. Let's wait for the CI builds.

Holzhaus · 2019-08-23T14:33:01Z

build/depends.py

@@ -197,15 +199,16 @@ def uic(build):
    @staticmethod
    def find_framework_libdir(qtdir):
        # Try pkg-config on Linux
-        import sys
-        if sys.platform.startswith('linux') or sys.platform.find('bsd') >= 0:


Looks like we musn't remove this line, because it makes the MacOS build fail...

uklotzde · 2019-08-23T14:48:46Z

Thank you for the detailed comments about how map() is supposed to work! This is not obvious for someone with hardly any Python knowledge like me.

Be-ing · 2019-08-23T14:48:51Z

I confirm this works on Fedora 30 with the scons-3 binary. 👍

Be-ing · 2019-08-23T14:49:50Z

I'm always afraid if we need to touch this brittle and hand-crafted build system.

FWIW, Qt is finally dropping their hand-crafted build system for Qt 6 and switching to CMake. We may consider doing the same soon.

Holzhaus · 2019-08-23T14:52:36Z

@uklotzde Sorry that I need to bother you with so many changes, but I do not have MacOS and it looks like I can't I start builds on Travis CI manually without opening my own PR.

@Be-ing

I'm always afraid if we need to touch this brittle and hand-crafted build system.

FWIW, Qt is finally dropping their hand-crafted build system for Qt 6 and switching to CMake. We may consider doing the same soon.

Sounds like a plan 👍

uklotzde · 2019-08-23T14:52:47Z

I don't want to switch to master for fc31/rawhide in RPM Fusion right now. But as soon as we branch out 2.3 we can remove the Python 2 workaround there.

Be-ing · 2019-08-23T14:57:44Z

Sounds like a plan +1

Are you volunteering to work on it? :)

uklotzde · 2019-08-23T14:57:57Z

Don't mind. You are doing great work!

I think if you register your own account on Travis you can build individual branches from your GitHub repositories. I have both AppVeyor and Travis accounts, just in case. But most of the time I'm too lazy and open some [WiP] PR if it's almost ready.

Be-ing · 2019-08-23T14:58:48Z

I think if you register your own account on Travis you can build individual branches from your GitHub repositories. I have both AppVeyor and Travis accounts, just in case. But most of the time I'm too lazy and open some [WiP] PR if it's almost ready.

We have a wiki page for that.

Holzhaus · 2019-08-23T15:05:47Z

I think if you register your own account on Travis you can build individual branches from your GitHub repositories. I have both AppVeyor and Travis accounts, just in case. But most of the time I'm too lazy and open some [WiP] PR if it's almost ready.

We have a wiki page for that.

Oops, I should RTFM in the future ;-)

Sounds like a plan +1

Are you volunteering to work on it? :)

I can't promise it, but the current SCons setup is super confusing so I probably should 😄

Holzhaus · 2019-08-23T15:25:19Z

Looks like the MacOS build finally succeeded.

uklotzde · 2019-08-23T16:37:57Z

The Windows build timed out, but otherwise it worked flawlessly. Let's wait another 30 minutes for the Ubuntu build, although I don't expect any new issues.

uklotzde · 2019-08-23T17:14:46Z

LGTM.

Since @Holzhaus has actually done most of the work I think I'm entitled to merge my own PR ;) Finally we are ready for Python 3 / SCons 3!

Holzhaus reviewed Jul 8, 2019

View reviewed changes

uklotzde changed the title ~~SCons: Decode stdout from subprocess~~ [WiP] SCons: Decode stdout from subprocess Jul 8, 2019

uklotzde changed the title ~~[WiP] SCons: Decode stdout from subprocess~~ SCons: Decode stdout from subprocess Jul 14, 2019

uklotzde added this to the 2.3.0 milestone Jul 15, 2019

uklotzde added 3 commits July 20, 2019 10:06

Use subprocess.checkout_output()

b1b7ad5

Fix compiler check for MSVC toolchain

3a5c5de

Remove redundant decode() args

abb345a

Holzhaus added 3 commits August 11, 2019 17:06

scons: Move stdlib imports to top of file

2b956c0

Imports that are not at the top of file are considered bad style in Python. See this for details: https://legacy.python.org/dev/peps/pep-0008/#imports

build: Improve pkg-config invocation

ecfbad4

scons: Remove unused imports

52de841

scons: Restore Scons 3.1.1 compatibility

d3562f1

The additional repo_node kwarg is required since this commit: SCons/scons@f999d21#diff-ac9db0da66287699ffbaf33c370d2c85R1508

uklotzde and others added 3 commits August 14, 2019 21:46

Merge branch 'master' of git@github.com:mixxxdj/mixxx.git into scons

ad8cddc

scons: Print SCons/Python version to make debugging easier

bb75e59

scons: Restore Python 2.7 compatibility

71b2374

This re-adds 'utf-8' to bytes.decode() calls and replaces FileNotFoundError with OSError to restore compatibility with Python 2.7. When Python 2.7 finally reaches EOL we can simply revert this commit.

scons: Also fix Python 2.7 compatibility in depends.py/qt5.py

5209ff7

scons: Fix UIC invocation via map() in Python 3

03fad27

Holzhaus reviewed Aug 23, 2019

View reviewed changes

scons: Catch CalledProcessErrors to allow fallback to defaults

8507218

uklotzde merged commit 9d02ef3 into mixxxdj:master Aug 23, 2019

uklotzde deleted the scons branch September 1, 2019 15:26

mixxxbot mentioned this pull request Aug 23, 2022

Compile issues with SCons 3 / Python 3 #9603

Closed

SCons: Decode stdout from subprocess #2201

SCons: Decode stdout from subprocess #2201

Conversation

uklotzde commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 14, 2019

Holzhaus commented Jul 20, 2019

rrrapha commented Jul 20, 2019

daschuer commented Jul 21, 2019

uklotzde commented Jul 22, 2019

Holzhaus commented Aug 11, 2019

uklotzde commented Aug 11, 2019

Holzhaus commented Aug 11, 2019

Be-ing commented Aug 11, 2019

Holzhaus commented Aug 12, 2019

Holzhaus commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

uklotzde commented Aug 23, 2019

uklotzde commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

uklotzde commented Aug 23, 2019

uklotzde commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

uklotzde commented Aug 23, 2019

Choose a reason for hiding this comment

uklotzde commented Aug 23, 2019

Be-ing commented Aug 23, 2019

Be-ing commented Aug 23, 2019

Holzhaus commented Aug 23, 2019 • edited Loading

uklotzde commented Aug 23, 2019

Be-ing commented Aug 23, 2019

uklotzde commented Aug 23, 2019

Be-ing commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

Holzhaus commented Aug 23, 2019

uklotzde commented Aug 23, 2019

uklotzde commented Aug 23, 2019

Holzhaus Jul 8, 2019 •

edited

Loading

Holzhaus commented Aug 23, 2019 •

edited

Loading