Copilot commented on code in PR #743:
URL: https://github.com/apache/tsfile/pull/743#discussion_r2935023941
##########
pom.xml:
##########
@@ -135,6 +135,10 @@
<exclude>**/tsfile.egg-info/**</exclude>
<!-- Exclude third_party-->
<exclude>**/third_party/**</exclude>
+ <exclude>**/.python-version</exclude>
+ <exclude>**/**venv-py**/**</exclude>
+ <exclude>**/.python-version</exclude>
+ <exclude>python/.python-version</exclude>
Review Comment:
There are duplicated/overlapping RAT excludes here (e.g.,
`**/.python-version` is listed twice, plus `python/.python-version`
separately), and the `**/**venv-py**/**` pattern is unusual and doesn't match
any path in the repo. Please deduplicate these entries and use a more precise
glob (e.g., `**/venv-py*/**` if that's the intent) to keep the build config
clear.
##########
python/setup.py:
##########
@@ -19,151 +19,134 @@
import os
import platform
import shutil
-
+import sys
import numpy as np
+
+from pathlib import Path
from Cython.Build import cythonize
from setuptools import setup, Extension
from setuptools.command.build_ext import build_ext
+ROOT = Path(__file__).parent.resolve()
+PKG = ROOT / "tsfile"
+CPP_OUT = ROOT / ".." / "cpp" / "target" / "build"
+CPP_LIB = CPP_OUT / "lib"
+CPP_INC = CPP_OUT / "include"
+
version = "2.2.1.dev"
system = platform.system()
-
-def copy_tsfile_lib(source_dir, target_dir, suffix):
- lib_file_name = f"libtsfile.{suffix}"
- source = os.path.join(source_dir, lib_file_name)
- target = os.path.join(target_dir, lib_file_name)
-
- if os.path.exists(source):
- shutil.copyfile(source, target)
-
- if system == "Linux":
- link_name = os.path.join(target_dir, "libtsfile.so")
- if os.path.exists(link_name):
- os.remove(link_name)
- os.symlink(lib_file_name, link_name)
- elif system == "Darwin":
- link_name = os.path.join(target_dir, "libtsfile.dylib")
- if os.path.exists(link_name):
- os.remove(link_name)
- os.symlink(lib_file_name, link_name)
-
-
-project_dir = os.path.dirname(os.path.abspath(__file__))
-tsfile_py_include = os.path.join(project_dir, "tsfile", "include")
-
-if os.path.exists(tsfile_py_include):
- shutil.rmtree(tsfile_py_include)
-
-shutil.copytree(
- os.path.join(project_dir, "..", "cpp", "target", "build", "include"),
- os.path.join(tsfile_py_include, ""),
-)
-
-
-def copy_tsfile_header(source):
- for file in source:
- if os.path.exists(file):
- target = os.path.join(tsfile_py_include, os.path.basename(file))
- shutil.copyfile(file, target)
-
-
-## Copy C wrapper header.
-# tsfile/cpp/src/cwrapper/tsfile_cwrapper.h
-source_headers = [
- os.path.join(project_dir, "..", "cpp", "src", "cwrapper",
"tsfile_cwrapper.h"),
-]
-
-copy_tsfile_header(source_headers)
-
-## Copy shared library
-tsfile_shared_source_dir = os.path.join(project_dir, "..", "cpp", "target",
"build", "lib")
-tsfile_shared_dir = os.path.join(project_dir, "tsfile")
-
-if system == "Darwin":
- copy_tsfile_lib(tsfile_shared_source_dir, tsfile_shared_dir, version +
".dylib")
-elif system == "Linux":
- copy_tsfile_lib(tsfile_shared_source_dir, tsfile_shared_dir, "so." +
version)
+(PKG / "include").mkdir(exist_ok=True)
+if (PKG / "include").exists() and CPP_INC.exists():
+ shutil.rmtree(PKG / "include")
+ shutil.copytree(CPP_INC, PKG / "include")
Review Comment:
If `cpp/target/build/include` is missing, this script currently leaves an
empty `tsfile/include` directory and proceeds, which will later fail during
compilation with less clear errors. Consider failing fast here (e.g., raise a
`FileNotFoundError` when `CPP_INC` does not exist) so packaging errors are
diagnosed early.
##########
.github/workflows/unit-test-python.yml:
##########
@@ -60,6 +65,17 @@ jobs:
key: ${{ runner.os }}-m2-${{ hashFiles('**/pom.xml') }}
restore-keys: ${{ runner.os }}-m2-
+ - name: Install dependencies
+ shell: bash
+ run: |
+ if [[ "$RUNNER_OS" == "Linux" ]]; then
+ if command -v apt-get >/dev/null 2>&1; then
+ sudo apt-get update
+ sudo apt-get install -y uuid-dev
+ elif command -v yum >/dev/null 2>&1; then
+ sudo yum install -y libuuid-devel
+ fi
+ fi
Review Comment:
This newly-added dependency install step is redundant with the existing
"Install dependencies" step below (which already installs `uuid-dev` on Linux).
Having two similarly-named steps makes the workflow harder to maintain;
consider merging them into a single step (and keep the package-manager
branching in one place).
##########
python/setup.py:
##########
@@ -19,151 +19,134 @@
import os
import platform
import shutil
-
+import sys
import numpy as np
+
+from pathlib import Path
from Cython.Build import cythonize
from setuptools import setup, Extension
from setuptools.command.build_ext import build_ext
+ROOT = Path(__file__).parent.resolve()
+PKG = ROOT / "tsfile"
+CPP_OUT = ROOT / ".." / "cpp" / "target" / "build"
+CPP_LIB = CPP_OUT / "lib"
+CPP_INC = CPP_OUT / "include"
+
version = "2.2.1.dev"
system = platform.system()
-
-def copy_tsfile_lib(source_dir, target_dir, suffix):
- lib_file_name = f"libtsfile.{suffix}"
- source = os.path.join(source_dir, lib_file_name)
- target = os.path.join(target_dir, lib_file_name)
-
- if os.path.exists(source):
- shutil.copyfile(source, target)
-
- if system == "Linux":
- link_name = os.path.join(target_dir, "libtsfile.so")
- if os.path.exists(link_name):
- os.remove(link_name)
- os.symlink(lib_file_name, link_name)
- elif system == "Darwin":
- link_name = os.path.join(target_dir, "libtsfile.dylib")
- if os.path.exists(link_name):
- os.remove(link_name)
- os.symlink(lib_file_name, link_name)
-
-
-project_dir = os.path.dirname(os.path.abspath(__file__))
-tsfile_py_include = os.path.join(project_dir, "tsfile", "include")
-
-if os.path.exists(tsfile_py_include):
- shutil.rmtree(tsfile_py_include)
-
-shutil.copytree(
- os.path.join(project_dir, "..", "cpp", "target", "build", "include"),
- os.path.join(tsfile_py_include, ""),
-)
-
-
-def copy_tsfile_header(source):
- for file in source:
- if os.path.exists(file):
- target = os.path.join(tsfile_py_include, os.path.basename(file))
- shutil.copyfile(file, target)
-
-
-## Copy C wrapper header.
-# tsfile/cpp/src/cwrapper/tsfile_cwrapper.h
-source_headers = [
- os.path.join(project_dir, "..", "cpp", "src", "cwrapper",
"tsfile_cwrapper.h"),
-]
-
-copy_tsfile_header(source_headers)
-
-## Copy shared library
-tsfile_shared_source_dir = os.path.join(project_dir, "..", "cpp", "target",
"build", "lib")
-tsfile_shared_dir = os.path.join(project_dir, "tsfile")
-
-if system == "Darwin":
- copy_tsfile_lib(tsfile_shared_source_dir, tsfile_shared_dir, version +
".dylib")
-elif system == "Linux":
- copy_tsfile_lib(tsfile_shared_source_dir, tsfile_shared_dir, "so." +
version)
+(PKG / "include").mkdir(exist_ok=True)
+if (PKG / "include").exists() and CPP_INC.exists():
+ shutil.rmtree(PKG / "include")
+ shutil.copytree(CPP_INC, PKG / "include")
+if sys.platform.startswith("linux"):
+ candidates = sorted(CPP_LIB.glob("libtsfile.so*"), key=lambda p:
len(p.name), reverse=True)
+ if not candidates:
+ raise FileNotFoundError("missing libtsfile.so* in build output")
+ src = candidates[0]
+ dst = PKG / src.name
+ shutil.copy2(src, dst)
+ link_name = PKG / "libtsfile.so"
+ shutil.copy2(src, link_name)
+
+elif sys.platform == "darwin":
+ candidates = sorted(CPP_LIB.glob("libtsfile.*.dylib")) or
list(CPP_LIB.glob("libtsfile.dylib"))
+ if not candidates:
+ raise FileNotFoundError("missing libtsfile*.dylib in build output")
+ src = candidates[0]
+ dst = PKG / src.name
+ shutil.copy2(src, dst)
+ link_name = PKG / "libtsfile.dylib"
+ shutil.copy2(src, link_name)
+elif sys.platform == "win32":
+ for base_name in ("libtsfile",):
+ dll_candidates = sorted(CPP_LIB.glob(f"{base_name}*.dll"), key=lambda
p: len(p.name), reverse=True)
+ dll_a_candidates = sorted(CPP_LIB.glob(f"{base_name}*.dll.a"),
key=lambda p: len(p.name), reverse=True)
+
+ if not dll_candidates:
+ raise FileNotFoundError(f"missing {base_name}*.dll in build
output")
+ if not dll_a_candidates:
+ raise FileNotFoundError(f"missing {base_name}*.dll.a in build
output")
+
+ dll_src = dll_candidates[0]
+ dll_a_src = dll_a_candidates[0]
+
+ shutil.copy2(dll_src, PKG / f"{base_name}.dll")
+ shutil.copy2(dll_a_src, PKG / f"{base_name}.dll.a")
+
+ # Copy MinGW runtime DLLs next to libtsfile.dll so Python can find them.
+ # Python 3.8+ does not search PATH for DLLs; they must be in the same
+ # directory as the .pyd extensions (registered via os.add_dll_directory).
+ for _mingw_dll in ("libstdc++-6.dll", "libgcc_s_seh-1.dll",
"libwinpthread-1.dll"):
+ for _dir in os.environ.get("PATH", "").split(os.pathsep):
+ _src = Path(_dir) / _mingw_dll
+ if _src.is_file():
+ shutil.copy2(_src, PKG / _mingw_dll)
+ print(f"setup.py: copied {_mingw_dll} from {_src}")
+ break
+ else:
+ print(f"setup.py: WARNING - {_mingw_dll} not found on PATH")
else:
- copy_tsfile_lib(tsfile_shared_source_dir, tsfile_shared_dir, "dll")
-
-tsfile_include_dir = os.path.join(project_dir, "tsfile", "include")
-
-ext_modules_tsfile = [
- # utils: from python to c or c to python.
- Extension(
- "tsfile.tsfile_py_cpp",
- sources=[os.path.join("tsfile", "tsfile_py_cpp.pyx")],
- libraries=["tsfile"],
- library_dirs=[tsfile_shared_dir],
- include_dirs=[tsfile_include_dir, np.get_include()],
- runtime_library_dirs=[tsfile_shared_dir] if system != "Windows" else
None,
- extra_compile_args=(
- ["-std=c++11"] if system != "Windows" else ["-std=c++11",
"-DMS_WIN64"]
- ),
- language="c++",
- ),
- # query data and describe schema: tsfile reader module
- Extension(
- "tsfile.tsfile_reader",
- sources=[os.path.join("tsfile", "tsfile_reader.pyx")],
- libraries=["tsfile"],
- library_dirs=[tsfile_shared_dir],
- depends=[os.path.join("tsfile", "tsfile_py_cpp.pxd")],
- include_dirs=[tsfile_include_dir, np.get_include()],
- runtime_library_dirs=[tsfile_shared_dir] if system != "Windows" else
None,
- extra_compile_args=(
- ["-std=c++11"] if system != "Windows" else ["-std=c++11",
"-DMS_WIN64"]
- ),
- language="c++",
- ),
- # write data and register schema: tsfile writer module
- Extension(
- "tsfile.tsfile_writer",
- sources=[os.path.join("tsfile", "tsfile_writer.pyx")],
- libraries=["tsfile"],
- library_dirs=[tsfile_shared_dir],
- depends=[os.path.join("tsfile", "tsfile_py_cpp.pxd")],
- include_dirs=[tsfile_include_dir, np.get_include()],
- runtime_library_dirs=[tsfile_shared_dir] if system != "Windows" else
None,
- extra_compile_args=(
- ["-std=c++11"] if system != "Windows" else ["-std=c++11",
"-DMS_WIN64"]
- ),
- language="c++",
- )
-]
+ raise RuntimeError(f"Unsupported platform: {sys.platform}")
class BuildExt(build_ext):
- def build_extensions(self):
- numpy_include = np.get_include()
- for ext in self.extensions:
- ext.include_dirs.append(numpy_include)
- super().build_extensions()
+ def run(self):
+ super().run()
def finalize_options(self):
- if system == "Windows":
+ if sys.platform == "win32":
self.compiler = "mingw32"
super().finalize_options()
+extra_compile_args = []
+extra_link_args = []
+runtime_library_dirs = []
+libraries = []
+library_dirs = [str(PKG)]
+include_dirs = [str(PKG), np.get_include(), str(PKG / "include")]
+
+if sys.platform.startswith("linux"):
+ libraries = ["tsfile"]
+ extra_compile_args += ["-O3", "-std=c++11", "-fvisibility=hidden", "-fPIC"]
+ runtime_library_dirs = ["$ORIGIN"]
+ extra_link_args += ["-Wl,-rpath,$ORIGIN"]
+elif sys.platform == "darwin":
+ libraries = ["tsfile"]
+ extra_compile_args += ["-O3", "-std=c++11", "-fvisibility=hidden", "-fPIC"]
+ extra_link_args += ["-Wl,-rpath,@loader_path", "-stdlib=libc++"]
+elif sys.platform == "win32":
+ libraries = ["Tsfile"]
Review Comment:
On Windows, `libraries = ["Tsfile"]` is inconsistent with the
produced/copying artifacts (`libtsfile.dll` and `libtsfile.dll.a`) and with the
previous cross-platform setup that linked against `tsfile`. This is likely to
break linking (MinGW typically looks for `lib<name>*.a`/`.dll.a` based on the
exact `-l<name>`). Align the library name with the actual import library
(usually `tsfile`).
##########
pom.xml:
##########
@@ -751,8 +757,8 @@
<os.suffix>win</os.suffix>
<os.classifier>windows-amd64</os.classifier>
<cmake.generator>MinGW Makefiles</cmake.generator>
- <python.venv.bin>venv/Scripts/</python.venv.bin>
- <python.exe.bin>python</python.exe.bin>
+ <python.venv.bin/>
Review Comment:
Setting `<python.venv.bin/>` for the `windows-amd64` profile means the
Python module build (see `python/pom.xml`) will create a venv but then run
pip/build/test commands against the *system* Python, and `pytest` will be
invoked from PATH. This can break builds on machines where global pip installs
are restricted and makes behavior inconsistent with other platforms. Consider
keeping `python.venv.bin` as `venv/Scripts/` and only switching
`python.exe.bin` to `python.exe`, or otherwise updating the Python module build
to not create/use a venv for this profile.
##########
python/tsfile/__init__.py:
##########
@@ -19,9 +19,16 @@
import ctypes
import os
import platform
Review Comment:
`platform` is imported but never used after switching to `sys.platform`;
please drop the unused import to avoid confusion and keep module imports
minimal.
##########
python/setup.py:
##########
@@ -19,151 +19,134 @@
import os
import platform
import shutil
-
+import sys
import numpy as np
+
+from pathlib import Path
from Cython.Build import cythonize
from setuptools import setup, Extension
from setuptools.command.build_ext import build_ext
+ROOT = Path(__file__).parent.resolve()
+PKG = ROOT / "tsfile"
+CPP_OUT = ROOT / ".." / "cpp" / "target" / "build"
+CPP_LIB = CPP_OUT / "lib"
+CPP_INC = CPP_OUT / "include"
+
version = "2.2.1.dev"
system = platform.system()
Review Comment:
`platform`/`system = platform.system()` are no longer used (the script
branches on `sys.platform`), so they should be removed to avoid dead code and
keep the build script easier to maintain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
