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

Fixed for msys/cygwin environments #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Evolver
Copy link

@Evolver Evolver commented Oct 5, 2017

Replaced commands.getstatusoutput() with subprocess.check_output() since the latter properly escapes command line arguments in msys environment.

Changed win32/unix detection to rely on presence of "uname", to cover msys/cygwin environments. Not sure if this would be the best way to detect unix shell, but the problem is that sys.platform returns "win32" in msys case, tricking the setup script into performing unnecessary escaping of MODULE_NAME macro.

…nce the latter properly escapes command line arguments in msys environment.

Changed win32/unix detection to rely on presence of "uname", to cover msys/cygwin environments. sys.platform returns "win32" in msys case, tricking the setup script into performing unnecessary escaping of MODULE_NAME macro.
@@ -68,7 +68,13 @@
It is almost fully compliant with the Python database API version 2.0 also
exposes the unique features of SQLite."""

if sys.platform != "win32":
try:
subprocess.check_output(["uname", "-a"])
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no specific reason to use check_output() vs check_call(). check_call() would actually be more preferable.

The "-a" argument also is excessive and can be removed.

@@ -29,7 +29,7 @@
print("Only Python 2.7 is supported.")
sys.exit(1)

import commands
import subprocess
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subprocess came in 2.4 (https://docs.python.org/2/library/subprocess.html#module-subprocess), so not sure how backwards compatible this change would be. Not following pysqlite history, so no idea if this is something to worry about.

@@ -122,8 +128,7 @@ class MyBuildExt(build_ext):
amalgamation = False

def _pkgconfig(self, flag, package):
status, output = commands.getstatusoutput("pkg-config %s %s" % (flag, package))
Copy link
Author

@Evolver Evolver Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was giving me "{ is not a recognized command" or something like that under msys. Looked up getstatusoutput (https://docs.python.org/2/library/commands.html) and they say this:

cmd is actually run as { cmd ; } 2>&1, so that the returned output will contain output or error messages

So that would be where the "{" "not found" comes from.

@@ -68,7 +68,13 @@
It is almost fully compliant with the Python database API version 2.0 also
exposes the unique features of SQLite."""

if sys.platform != "win32":
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under msys you end up with "win32" here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant