ovalhub / pyicu

The PyICU project repository has moved to https://pyicu.org.
Other
133 stars 49 forks source link

Fix build error on Python 3.9 for Windows #136

Closed cgohlke closed 4 years ago

cgohlke commented 4 years ago

Fixes

_icu.cpp(209): error C2017: illegal escape sequence
_icu.cpp(209): error C2001: newline in constant
_icu.cpp(210): error C2146: syntax error: missing ')' before identifier 'PyObject_SetAttrString'
_icu.cpp(210): error C2146: syntax error: missing ';' before identifier 'PyObject_SetAttrString'
_icu.cpp(212): error C2017: illegal escape sequence
_icu.cpp(212): error C2001: newline in constant
_icu.cpp(213): error C2146: syntax error: missing ')' before identifier 'PyObject_SetAttrString'
_icu.cpp(213): error C2146: syntax error: missing ';' before identifier 'PyObject_SetAttrString'
ovalhub commented 4 years ago

I don't understand what this path fixes ? You're changing VER_FLAGS['win32'] from ['/DPYICU_VER=\"%s\"' %(VERSION), '/DPYICU_ICU_MAX_VER=\"%s\"' %(ICU_MAX_MAJOR_VERSION)] to ['-DPYICU_VER="%s"' %(VERSION), '-DPYICU_ICU_MAX_VER="%s"' %(ICU_MAX_MAJOR_VERSION)]

if Python's version >= 3.9. Can you please explain what the intent here is ? It can't be that Python has any influence over the syntax the MSVC C++ compiler accepts ?

Thanks !

Andi..

On Thu, 30 Jul 2020, Christoph Gohlke wrote:

Fixes

_icu.cpp(209): error C2017: illegal escape sequence
_icu.cpp(209): error C2001: newline in constant
_icu.cpp(210): error C2146: syntax error: missing ')' before identifier 'PyObject_SetAttrString'
_icu.cpp(210): error C2146: syntax error: missing ';' before identifier 'PyObject_SetAttrString'
_icu.cpp(212): error C2017: illegal escape sequence
_icu.cpp(212): error C2001: newline in constant
_icu.cpp(213): error C2146: syntax error: missing ')' before identifier 'PyObject_SetAttrString'
_icu.cpp(213): error C2146: syntax error: missing ';' before identifier 'PyObject_SetAttrString'

You can view, comment on, or merge this pull request online at:

https://github.com/ovalhub/pyicu/pull/136

-- Commit Summary --

  • Fix build error on Python 3.9 for Windows

-- File Changes --

M setup.py (6)

-- Patch Links --

https://github.com/ovalhub/pyicu/pull/136.patch https://github.com/ovalhub/pyicu/pull/136.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/ovalhub/pyicu/pull/136

cgohlke commented 4 years ago

Apparently Python 3.9 changed/fixed the way it passes extra_compile_args on to the command line. The spawn module was largely rewritten.

ovalhub commented 4 years ago

On Thu, 30 Jul 2020, Christoph Gohlke wrote:

Apparently Python 3.9 changed/fixed the way it passes extra_compile_args on to the command line. The spawn module was largely rewritten.

Oh, wow, nice breakage for everyone. But if we don't have to mess with '/' command line flags for Windows's sake that's progress too. I'll probably integrate it so that the flags not duplicated, just the '/' vs '-' conditionalized.

Thank you for the fix ! Andi..

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/ovalhub/pyicu/pull/136#issuecomment-666874935

ovalhub commented 4 years ago

On Thu, 30 Jul 2020, Christoph Gohlke wrote:

Apparently Python 3.9 changed/fixed the way it passes extra_compile_args on to the command line. The spawn module was largely rewritten.

Oh, I just noticed that you're also dropping the \ in front of the '"'. This is not needed anymore either ?

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/ovalhub/pyicu/pull/136#issuecomment-666874935

cgohlke commented 4 years ago

Sorry for the confusion: replacing /D with -D was unintentional (I can revert that change if you prefer). The issue was the escape of the quotes " vs \\".

ovalhub commented 4 years ago

On Thu, 30 Jul 2020, Christoph Gohlke wrote:

Sorry for the confusion: replacing /D with -D was unintentional (I can revert that change if you prefer). The issue was the escape of the quotes " vs \\".

Ah, interesting. No, if '-' works, I'd rather use that. I just now changed this code to:

if sys.platform == 'win32' and sys.version_info < (3,9): ver_flag = '/D%s=\"%s\"' else: ver_flag = '-D%s="%s"'

VER_FLAGS = [ver_flag %('PYICU_VER', VERSION), ver_flag %('PYICU_ICU_MAX_VER', ICU_MAX_MAJOR_VERSION)]

(not yet checked in)

Andi..

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/ovalhub/pyicu/pull/136#issuecomment-666882447

ovalhub commented 4 years ago

Thank you, I ended up simplifying this area of the code while integrating your patch.