kou commented on code in PR #41721: URL: https://github.com/apache/arrow/pull/41721#discussion_r1606875143
########## c_glib/tools/gen-version-header.py: ########## Review Comment: Just to confirm, is this written from scratch? ########## c_glib/parquet-glib/version.h.in: ########## @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <arrow-glib/version.h> Review Comment: Do we need include this? ########## c_glib/tools/gen-version-header.py: ########## Review Comment: Could you enable formatter? ```diff diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index bf5ca08d53..d4a9ca6602 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -40,7 +40,7 @@ repos: hooks: - id: flake8 name: Python Format - files: ^(python|dev|integration)/ + files: ^(python|dev|glib|integration)/ types: - file - python ``` ########## c_glib/tools/gen-version-header.py: ########## @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +import argparse +from io import TextIOBase +from pathlib import Path + + +def main(): + parser = argparse.ArgumentParser( + description="Generate C header with version macros") + parser.add_argument( + "--library", + required=True, + help="The library name to use in macro prefixes") + parser.add_argument( + "--version", + required=True, + help="The library version number") + parser.add_argument( + "--input", + type=Path, + required=True, + help="Path to the input template file") + parser.add_argument( + "--output", + type=Path, + required=True, + help="Path to the output file to generate") + parser.add_argument( + "--version-library", + default="GARROW", + help="The library name prefix to use in MIN_REQUIRED and MAX_ALLOWED checks") + + args = parser.parse_args() + with open(args.input, "r", encoding="utf-8") as input_file, \ + open(args.output, "w", encoding="utf-8") as output_file: + write_header(input_file, output_file, args.library, args.version, args.version_library) + + +def write_header( + input_file: TextIOBase, + output_file: TextIOBase, + library_name: str, + version: str, + version_library: str): + if "-" in version: + version, version_tag = version.split("-") + else: + version_tag = "" + version_major, version_minor, version_micro = [int(v) for v in version.split(".")] + + availability_macros = generate_availability_macros(library_name, version_library) + + replacements = { + "VERSION_MAJOR": str(version_major), + "VERSION_MINOR": str(version_minor), + "VERSION_MICRO": str(version_micro), + "VERSION_TAG": version_tag, + "AVAILABILITY_MACROS": availability_macros, + } + + replacements = dict( + (f"@{key}@", replacement) for (key, replacement) in replacements.items()) + + for line in input_file: + if "@" in line: + for key, replacement in replacements.items(): + line = line.replace(key, replacement) + output_file.write(line) + + +def generate_availability_macros(library: str, version_library: str) -> str: + versions = [ + (16, 0), Review Comment: We should update this list automatically in release process: ```diff diff --git a/dev/release/utils-prepare.sh b/dev/release/utils-prepare.sh index 5136708722..af1172e50b 100644 --- a/dev/release/utils-prepare.sh +++ b/dev/release/utils-prepare.sh @@ -40,6 +40,14 @@ update_versions() { meson.build rm -f meson.build.bak git add meson.build + + if [ "${type}" = "snapshot" ]; then + sed -i.bak -E -e \ + "s/^ versions =/a\ (${major_version}, 0)" \ + tools/gen-version-header.py + rm -f tools/gen-version-header.py.bak + git add tools/gen-version-header.py + fi popd pushd "${ARROW_DIR}/ci/scripts" ``` ########## c_glib/tools/gen-version-header.py: ########## @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +import argparse +from io import TextIOBase +from pathlib import Path + + +def main(): + parser = argparse.ArgumentParser( + description="Generate C header with version macros") + parser.add_argument( + "--library", + required=True, + help="The library name to use in macro prefixes") + parser.add_argument( + "--version", + required=True, + help="The library version number") + parser.add_argument( + "--input", + type=Path, + required=True, + help="Path to the input template file") + parser.add_argument( + "--output", + type=Path, + required=True, + help="Path to the output file to generate") + parser.add_argument( + "--version-library", + default="GARROW", + help="The library name prefix to use in MIN_REQUIRED and MAX_ALLOWED checks") + + args = parser.parse_args() + with open(args.input, "r", encoding="utf-8") as input_file, \ + open(args.output, "w", encoding="utf-8") as output_file: + write_header(input_file, output_file, args.library, args.version, args.version_library) + + +def write_header( + input_file: TextIOBase, + output_file: TextIOBase, + library_name: str, + version: str, + version_library: str): + if "-" in version: + version, version_tag = version.split("-") + else: + version_tag = "" + version_major, version_minor, version_micro = [int(v) for v in version.split(".")] + + availability_macros = generate_availability_macros(library_name, version_library) + + replacements = { + "VERSION_MAJOR": str(version_major), + "VERSION_MINOR": str(version_minor), + "VERSION_MICRO": str(version_micro), + "VERSION_TAG": version_tag, + "AVAILABILITY_MACROS": availability_macros, + } + + replacements = dict( + (f"@{key}@", replacement) for (key, replacement) in replacements.items()) + + for line in input_file: + if "@" in line: + for key, replacement in replacements.items(): + line = line.replace(key, replacement) + output_file.write(line) Review Comment: Can we simplify this something like the following? ```python output_file.write(re.sub('@(.+?)@', lambda match: replacements[match[1]], input_file.read())) ``` ########## c_glib/tools/gen-version-header.py: ########## @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +import argparse +from io import TextIOBase +from pathlib import Path + + +def main(): + parser = argparse.ArgumentParser( + description="Generate C header with version macros") + parser.add_argument( + "--library", + required=True, + help="The library name to use in macro prefixes") + parser.add_argument( + "--version", + required=True, + help="The library version number") + parser.add_argument( + "--input", + type=Path, + required=True, + help="Path to the input template file") + parser.add_argument( + "--output", + type=Path, + required=True, + help="Path to the output file to generate") + parser.add_argument( + "--version-library", + default="GARROW", Review Comment: How about requiring specifying this explicitly because we need to specify different value for each module? ```suggestion required=True, ``` ########## c_glib/arrow-glib/meson.build: ########## @@ -205,15 +205,16 @@ cpp_internal_headers = files( 'internal-index.hpp', ) -version_h_conf = configuration_data() -version_h_conf.set('GARROW_VERSION_MAJOR', version_major) -version_h_conf.set('GARROW_VERSION_MINOR', version_minor) -version_h_conf.set('GARROW_VERSION_MICRO', version_micro) -version_h_conf.set('GARROW_VERSION_TAG', version_tag) -version_h = configure_file(input: 'version.h.in', - output: 'version.h', - configuration: version_h_conf) -c_headers += version_h +version_h = custom_target( + 'arrow-glib-version-header', + input: 'version.h.in', + output: 'version.h', + command: [gen_version_header, '--library', 'GARROW', '--version', version, '--input', '@INPUT@', '--output', '@OUTPUT@'], + install: true, + install_dir: join_paths(include_dir, 'arrow-glib') +) + +built_headers = [version_h] Review Comment: Could you keep including `version.h` in `gnome.mkenums()` targets? ########## c_glib/arrow-dataset-glib/version.h.in: ########## @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <arrow-glib/version.h> Review Comment: Do we need this? ########## c_glib/arrow-flight-sql-glib/version.h.in: ########## @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <arrow-glib/version.h> Review Comment: Do we need this? ########## c_glib/meson.build: ########## @@ -171,6 +171,9 @@ if cxx.get_id() != 'msvc' endif add_project_arguments(cxx.get_supported_arguments(cxx_flags), language: 'cpp') +tools_dir = join_paths(project_source_root, 'tools') +gen_version_header = find_program('gen-version-header.py', dirs: [tools_dir]) Review Comment: How about find `python3` instead of `gen-version-header.py`? ```meson python = import('python') python3 = python.find_installation('python3') gen_version_header_py = project_source_root / 'tools' / 'gen-version-header.py' # ... command: [python3, gen_version_header_py, ...] ```` ########## c_glib/arrow-flight-glib/version.h.in: ########## @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <arrow-glib/version.h> Review Comment: Do we need this? ########## c_glib/tools/gen-version-header.py: ########## Review Comment: Could you rename this to `tool/generate-version-header.py`? 1. Existing directories don't have `s` 2. We don't need to abbreviate `gen` here ########## c_glib/arrow-cuda-glib/version.h.in: ########## @@ -0,0 +1,24 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include <arrow-glib/version.h> Review Comment: Do we need this? -- 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]
