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.

Sign up for GitHub

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

FIX: Fix or suppress the warning. #91

Merged
merged 4 commits into from
Dec 26, 2024
Merged

FIX: Fix or suppress the warning. #91

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e01c0a9
FIX: Fix or suppress the warning.
sabonerune Dec 20, 2024
36acdff
FIX: Revert to `cython>=0.28.0`
sabonerune Dec 25, 2024
d9e08a8
FIX: fix Cython version.
sabonerune Dec 26, 2024
723bdde
Merge branch 'master' into fix/fix-or-suppress-warning
r9y9 Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pyopenjtalk/htsengine.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# coding: utf-8
# cython: boundscheck=True, wraparound=True
# cython: c_string_type=unicode, c_string_encoding=ascii
# cython: language_level=3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cython 3 now warns about code that has no language-level.


import numpy as np

Expand All @@ -19,7 +20,7 @@ from .htsengine cimport (
HTS_Engine_get_generated_speech, HTS_Engine_get_nsamples
)

cdef class HTSEngine(object):
cdef class HTSEngine:
Comment on lines -36 to +37
Copy link
Contributor Author

@sabonerune sabonerune Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This can be made unnecessary by setting language_level=3.

https://cython.readthedocs.io/en/3.0.x/src/changes.html#id33

  • With language_level=3/3str, Python classes without explicit base class are now new-style (type) classes also in Py2. Previously, they were created as old-style (non-type) classes. (Github issue #3530)

(It doesn't change anything since Python 3 isn't supported in the first place.)

"""HTSEngine

Args:
Expand Down
3 changes: 2 additions & 1 deletion pyopenjtalk/htsengine/__init__.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3


cdef extern from "HTS_engine.h":
Expand All @@ -19,4 +20,4 @@ cdef extern from "HTS_engine.h":
size_t HTS_Engine_get_nsamples(HTS_Engine * engine)

void HTS_Engine_set_speed(HTS_Engine * engine, double f)
void HTS_Engine_add_half_tone(HTS_Engine * engine, double f)
void HTS_Engine_add_half_tone(HTS_Engine * engine, double f)
3 changes: 2 additions & 1 deletion pyopenjtalk/openjtalk.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# coding: utf-8
# cython: boundscheck=True, wraparound=True
# cython: c_string_type=unicode, c_string_encoding=ascii
# cython: language_level=3

import numpy as np

Expand Down Expand Up @@ -151,7 +152,7 @@ cdef inline int Mecab_load_with_userdic(Mecab *m, char* dicdir, char* userdic):
return 1


cdef class OpenJTalk(object):
cdef class OpenJTalk:
"""OpenJTalk

Args:
Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/__init__.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# cython: language_level=3
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/jpcommon.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

from libc.stdio cimport FILE

Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/mecab.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

cdef extern from "mecab.h":
cdef cppclass Mecab:
Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/mecab2njd.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

from .njd cimport NJD

Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/njd.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

from libc.stdio cimport FILE

Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/njd2jpcommon.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

from .jpcommon cimport JPCommon
from .njd cimport NJD
Expand Down
1 change: 1 addition & 0 deletions pyopenjtalk/openjtalk/text2mecab.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# distutils: language = c++
# cython: language_level=3

cdef extern from "text2mecab.h":
void text2mecab(char *output, const char *input)
4 changes: 2 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
requires = [
"setuptools>=64",
"setuptools_scm>=8",
"cython>=0.28.0",
"cython>=3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump to Cython>=3.

Copy link
Owner

Choose a reason for hiding this comment

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

It sounds reasonable to me for now, but we may consider reverting this change if some people complain about dependencies. I am generally reluctant to tighten dependencies unless it's strongly necessary.

By the way, what's the change that indeed requires cython >= 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/~https://github.com/r9y9/pyopenjtalk/pull/91/files#r1893870541

https://cython.readthedocs.io/en/3.0.x/src/userguide/migrating_to_cy30.html#numpy-c-api

Cython used to generate code that depended on the deprecated pre-NumPy-1.7 C-API. This is no longer the case with Cython 3.0.

Building with Cython<3 installed will fail.

Copy link
Owner

Choose a reason for hiding this comment

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

diff --git a/pyproject.toml b/pyproject.toml
index 0599f7a..0e27bb7 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -2,7 +2,7 @@
 requires = [
     "setuptools>=64",
     "setuptools_scm>=8",
-    "cython>=3",
+    "cython<3",
     "cmake",
     "numpy>=1.25.0; python_version>='3.9'",
     "oldest-supported-numpy; python_version<'3.9'",
@@ -52,7 +52,7 @@ docs = [
     "jupyter",
 ]
 dev = [
-    "cython>=3",
+    "cython<3",
     "pysen",
     "types-setuptools",
     "mypy<=0.910",

With this PR and above, it works OK in my environment

$ pip install -e . -vvv
...

    g++ -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I/Users/ryuichi/anaconda3/envs/py38/include -arch x86_64 -I/Users/ryuichi/anaconda3/envs/py38/include -arch x86_64 -bundle -undefined dynamic_lookup build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_audio.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_engine.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_gstream.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_label.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_misc.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_model.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_pstream.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_sstream.o build/temp.macosx-10.9-x86_64-cpython-38/lib/hts_engine_API/src/lib/HTS_vocoder.o build/temp.macosx-10.9-x86_64-cpython-38/pyopenjtalk/htsengine.o -o build/lib.macosx-10.9-x86_64-cpython-38/pyopenjtalk/htsengine.cpython-38-darwin.so
    copying build/lib.macosx-10.9-x86_64-cpython-38/pyopenjtalk/openjtalk.cpython-38-darwin.so -> pyopenjtalk
    copying build/lib.macosx-10.9-x86_64-cpython-38/pyopenjtalk/htsengine.cpython-38-darwin.so -> pyopenjtalk
    Creating /Users/ryuichi/anaconda3/envs/py38/lib/python3.8/site-packages/pyopenjtalk.egg-link (link to .)
    Adding pyopenjtalk 0.3.5.dev10+ge01c0a9.d20241225 to easy-install.pth file

    Installed /Users/ryuichi/Dropbox/sp/pyopenjtalk
Successfully installed pyopenjtalk-0.3.5.dev10+ge01c0a9.d20241225
Removed build tracker: '/private/var/folders/_z/zg2q619x6ql74chs2mm0wf9r0000gn/T/pip-req-tracker-2yy2p_0b'

As far as I understand, using cython <3 should be fine. Cython < 3 and newer numpy won't work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also confirmed that it works with Cython<3.
Revert to cython>=0.28.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, I noticed that Python 3.8 requires at least Cython 0.29.12.

The minimum version for which a wheel is provided is 0.29.14.
https://pypi.org/project/Cython/0.29.14/

The minimum version for which Programming Language::Python::3.8 exists in classifiers is 0.29.16.
https://pypi.org/project/Cython/0.29.16/

Copy link
Owner

Choose a reason for hiding this comment

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

Fixing requirements to "cython>=0.29.16" looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set to "cython>=0.29.16"

"cmake",
"numpy>=1.25.0; python_version>='3.9'",
"oldest-supported-numpy; python_version<'3.9'",
Expand Down Expand Up @@ -52,7 +52,7 @@ docs = [
"jupyter",
]
dev = [
"cython>=0.28.0",
"cython>=3",
"pysen",
"types-setuptools",
"mypy<=0.910",
Expand Down
16 changes: 16 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,18 @@ def msvc_extra_compile_args(compile_args):
return list(chain(compile_args, xs))


msvc_define_macros_config = [
("_CRT_NONSTDC_NO_WARNINGS", None),
("_CRT_SECURE_NO_WARNINGS", None),
]


def msvc_define_macros(macros):
mns = set([i[0] for i in macros])
xs = filter(lambda x: x[0] not in mns, msvc_define_macros_config)
return list(chain(macros, xs))
Comment on lines +26 to +35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSVC will warn for POSIX names.
It will also warn for functions that have alternatives in Annex K.

https://learn.microsoft.com/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-170#posix-function-names
https://learn.microsoft.com/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4996?view=msvc-170#unsafe-crt-library-functions

Suppresses the warning because it is impractical to write POSIX-compatible code without using these functions.



class custom_build_ext(setuptools.command.build_ext.build_ext):
def build_extensions(self):
compiler_type_is_msvc = self.compiler.compiler_type == "msvc"
Expand All @@ -33,6 +45,9 @@ def build_extensions(self):
if hasattr(entry, "extra_compile_args")
else []
)
entry.define_macros = msvc_define_macros(
entry.define_macros if hasattr(entry, "define_macros") else []
)

setuptools.command.build_ext.build_ext.build_extensions(self)

Expand Down Expand Up @@ -148,6 +163,7 @@ def check_cmake_in_path():
language="c++",
define_macros=[
("AUDIO_PLAY_NONE", None),
("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Cython 3.0 onwards, defining NPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION will prevent the deprecated Numpy C API from being used.
https://cython.readthedocs.io/en/3.0.x/src/userguide/migrating_to_cy30.html#numpy-c-api
https://cython.readthedocs.io/en/latest/src/userguide/numpy_tutorial.html#compilation-using-setuptools

It is also required for the pyopenjtalk.openjtalk module, but it will be omitted in this PR because it will no longer be necessary in #87.

],
)
]
Expand Down
Loading