GH-41134: [GLib] Support building arrow-glib with MSVC #41599

adamreeve · 2024-05-09T05:42:14Z

Rationale for this change

Allow Windows users to more easily build the GLib libraries.

What changes are included in this PR?

Minor fixes to allow building with MSVC:
- Changes some uses of variable length arrays to std::vector, because MSVC doesn't support variable length arrays
- Moves some function definitions that use C++ types outside of G_BEGIN_DECLS/G_END_DECLS blocks (which expand to extern C { }), because this caused MSVC to error
Fix libraries not having any exported symbols with MSVC, which defaults to hiding symbols
- Add visibility.h which defines a new GARROW_EXTERN macro that adds dllimport or dllexport attributes when using MSVC.
- Include the GARROW_EXTERN macro in the definitions of the GARROW_AVAILABLE_IN_* macros.
Add a new CI job that builds the GLib libraries with MSVC on Windows, using vcpkg to install pkgconfig and glib.
For now only arrow-glib is built, I can follow up with the other libraries after this PR. That will require introducing new per-library version macros.

Are these changes tested?

The build will be tested in CI but I've only done some quick manual tests that the built library works correctly, I haven't got the ruby tests running against the build yet.

Are there any user-facing changes?

No? Eventually some documentation should be updated when all the GLib libraries can be built with MSVC though

GitHub Issue: [GLib] Add support for MSVC with vcpkg #41134

github-actions · 2024-05-09T05:42:42Z

⚠️ GitHub issue #41134 has been automatically assigned in GitHub to PR creator.

kou

Thanks!

c_glib/arrow-glib/visibility.h

.github/workflows/ruby.yml

kou · 2024-05-10T01:50:54Z

.github/workflows/ruby.yml

+        env:
+          # We can invalidate the current cache by updating this.
+          CACHE_VERSION: "2024-05-09"
+      - name: Build


Suggested change

- name: Build

- name: Build C++

Can we also use vcpkg to install dependencies of C++?

I looked into this and got it working on another branch (https://github.com/adamreeve/arrow/tree/msvc_glib_vcpkg), but it's quite slow to build all the vcpkg dependencies (48 minutes on my GitHub actions run) and this isn't integrated with ccache so will be slow every time. If you think it's worth doing I can look at caching the vcpkg builds too? We do this in ParquetSharp so it shouldn't be too hard to add: https://github.com/G-Research/ParquetSharp/blob/ef8e7c447ecd5d84205eb73293162f5689e977b8/.github/workflows/ci.yml#L99-L118

OK. Let's work on it as a separated task.

FYI: We have a job that uses VCPKG_BINARY_SOURCES:

arrow/dev/tasks/vcpkg-tests/github.windows.yml

Line 27 in 657c4fa

VCPKG_BINARY_SOURCES: 'clear;nuget,GitHub,readwrite'

It may also work.

kou · 2024-05-10T02:06:01Z

c_glib/arrow-glib/array-builder.h

 GArrowArray *
 garrow_array_builder_finish(GArrowArrayBuilder *builder, GError **error);

 GARROW_AVAILABLE_IN_2_0
+GARROW_EXPORT


Let's add this macro to GARROW_AVAILABLE_IN_XXX. GLib does it.

This is what I was originally thinking of doing, but then I was worried about how this works with the different garrow libraries. We'd probably need separate available/deprecated macros per library like:

GARROW_AVAILABLE_IN_XXX GARROW_DEPRECATED_IN_XXX GARROW_DATASET_AVAILABLE_IN_XXX GARROW_DATASET_DEPRECATED_IN_XXX GARROW_FLIGHT_AVAILABLE_IN_XXX GARROW_FLIGHT_DEPRECATED_IN_XXX GARROW_FLIGHT_SQL_AVAILABLE_IN_XXX GARROW_FLIGHT_SQL_DEPRECATD_IN_XXX ...

That way when you build eg. arrow-dataset-glib and include headers from arrow-glib, they can correctly get the dllimport attribute while code in arrow-dataset-glib gets dllexport.

Maybe I should look at integrating that Python script from GLib to reduce the boilerplate required. What do you think?

Right. We need separated version macros per library.

I thought I could have this PR only change the arrow-glib library and do the others in a follow up to keep the changes manageable, but the MinGW build fails with link errors like "redeclared without dllimport attribute after being referenced with dll linkage", so I might need to add the separated version macros as part of this PR too, or make the visibility attributes MSVC only for now.

I've changed the macro definitions to only add dllimport/dllexport for MSVC for now, so this works as currently only the arrow-glib library is built for MSVC. But I can follow up to also add them under MinGW GCC later after adding version macros per-library. Does that work for you or would you rather include the per-library macros in this PR?

How about opening a separated PR that adds version macros per-library, merging it and rebasing on main?

(I'm OK with a follow up approach.)

I like that idea, I've made #41681 for this and will make a PR for that before updating this PR.

ci/scripts/install_vcpkg.sh

ci/scripts/c_glib_build.sh

c_glib/vcpkg.json

adamreeve · 2024-05-14T00:26:41Z

.github/workflows/ruby.yml

+      ARROW_FLIGHT: OFF
+      ARROW_FLIGHT_SQL: OFF
+      ARROW_HDFS: OFF
+      ARROW_HOME: "${{ github.workspace }}/dist"


I had to change ARROW_HOME to be in the workspace directory rather than /usr, as /usr becomes C:\Program Files\Git\usr as a Windows path and I was getting errors when linking like:

[47/54] Linking target arrow-glib/arrow-glib-1700.dll FAILED: arrow-glib/arrow-glib-1700.dll arrow-glib/arrow-glib-1700.pdb "link" @arrow-glib/arrow-glib-1700.dll.rsp LINK : fatal error LNK1181: cannot open input file 'Files\Git\usr\bin.obj'

I didn't get to the bottom of where this was going wrong but it seems like something in meson is failing to quote the linker command inputs.

raulcd · 2024-05-14T17:58:20Z

@github-actions crossbow submit -g release

github-actions · 2024-05-14T17:59:41Z

Invalid group(s) {'release'}. Must be one of {'homebrew', 'linux', 'example-cpp', 'verify-rc-wheels', 'packaging', 'c-glib', 'nightly-packaging', 'wheel', 'verify-rc-binaries', 'linux-arm64', 'python', 'vcpkg', 'linux-amd64', 'fuzz', 'java', 'verify-rc-source-macos', 'example-python', 'test', 'example', 'cpp', 'verify-rc-source', 'ruby', 'verify-rc', 'go', 'nightly-tests', 'conan', 'verify-rc-jars', 'r', 'verify-rc-source-linux', 'nightly-release', 'integration', 'nightly', 'conda'}
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/9083989638

raulcd · 2024-05-14T18:05:29Z

@github-actions crossbow submit -g nightly-release

github-actions · 2024-05-14T18:08:32Z

Revision: 4dc31f8

Submitted crossbow builds: ursacomputing/crossbow @ actions-012c96d84c

Task	Status
verify-rc-source-cpp-linux-almalinux-8-amd64
verify-rc-source-cpp-linux-conda-latest-amd64
verify-rc-source-cpp-linux-ubuntu-20.04-amd64
verify-rc-source-cpp-linux-ubuntu-22.04-amd64
verify-rc-source-cpp-macos-amd64
verify-rc-source-cpp-macos-arm64
verify-rc-source-cpp-macos-conda-amd64
verify-rc-source-csharp-linux-almalinux-8-amd64
verify-rc-source-csharp-linux-conda-latest-amd64
verify-rc-source-csharp-linux-ubuntu-20.04-amd64
verify-rc-source-csharp-linux-ubuntu-22.04-amd64
verify-rc-source-csharp-macos-amd64
verify-rc-source-csharp-macos-arm64
verify-rc-source-go-linux-almalinux-8-amd64
verify-rc-source-go-linux-conda-latest-amd64
verify-rc-source-go-linux-ubuntu-20.04-amd64
verify-rc-source-go-linux-ubuntu-22.04-amd64
verify-rc-source-go-macos-amd64
verify-rc-source-go-macos-arm64
verify-rc-source-integration-linux-almalinux-8-amd64
verify-rc-source-integration-linux-conda-latest-amd64
verify-rc-source-integration-linux-ubuntu-20.04-amd64
verify-rc-source-integration-linux-ubuntu-22.04-amd64
verify-rc-source-integration-macos-amd64
verify-rc-source-integration-macos-arm64
verify-rc-source-integration-macos-conda-amd64
verify-rc-source-java-linux-almalinux-8-amd64
verify-rc-source-java-linux-conda-latest-amd64
verify-rc-source-java-linux-ubuntu-20.04-amd64
verify-rc-source-java-linux-ubuntu-22.04-amd64
verify-rc-source-java-macos-amd64
verify-rc-source-js-linux-almalinux-8-amd64
verify-rc-source-js-linux-conda-latest-amd64
verify-rc-source-js-linux-ubuntu-20.04-amd64
verify-rc-source-js-linux-ubuntu-22.04-amd64
verify-rc-source-js-macos-amd64
verify-rc-source-js-macos-arm64
verify-rc-source-python-linux-almalinux-8-amd64
verify-rc-source-python-linux-conda-latest-amd64
verify-rc-source-python-linux-ubuntu-20.04-amd64
verify-rc-source-python-linux-ubuntu-22.04-amd64
verify-rc-source-python-macos-amd64
verify-rc-source-python-macos-arm64
verify-rc-source-python-macos-conda-amd64
verify-rc-source-ruby-linux-almalinux-8-amd64
verify-rc-source-ruby-linux-conda-latest-amd64
verify-rc-source-ruby-linux-ubuntu-20.04-amd64
verify-rc-source-ruby-linux-ubuntu-22.04-amd64
verify-rc-source-ruby-macos-amd64
verify-rc-source-ruby-macos-arm64
verify-rc-source-windows

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

adamreeve · 2024-05-23T03:03:46Z

I've rebased on top of the changes from #41721 to add per-library version headers, and added per-library visibility headers too. I've had to keep the Gandiva build disabled due to #36958 and also disabled the Flight libraries due to a missing zlib dependency, but I'm hoping that should be fixed by switching to vcpkg dependencies for the C++ library build.

kou · 2024-05-23T03:44:58Z

c_glib/tool/generate-visibility-header.py

+
+
+def write_header(output_file: TextIOBase, library: str):
+    output_file.write(f"""#pragma once


Could you also generate license header?
Or how about adding this to version.h instead of creating a separated visibility.h?

Yeah adding this to version.h makes sense, there isn't any good reason it needs to be separate.

github-actions bot added Component: GLib awaiting review Awaiting review labels May 9, 2024

adamreeve force-pushed the msvc_glib branch from 32bb181 to 9c2e23e Compare May 9, 2024 10:01

adamreeve changed the title ~~GH-41134: [GLib] Support building arrow-glib with MSVC~~ WIP: GH-41134: [GLib] Support building arrow-glib with MSVC May 9, 2024

kou reviewed May 10, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting changes Awaiting changes awaiting committer review Awaiting committer review labels May 10, 2024

adamreeve force-pushed the msvc_glib branch from f747702 to 12cc1d6 Compare May 10, 2024 10:47

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 10, 2024

adamreeve force-pushed the msvc_glib branch 3 times, most recently from b1ba360 to beffceb Compare May 13, 2024 23:18

adamreeve commented May 14, 2024

View reviewed changes

adamreeve changed the title ~~WIP: GH-41134: [GLib] Support building arrow-glib with MSVC~~ GH-41134: [GLib] Support building arrow-glib with MSVC May 14, 2024

adamreeve marked this pull request as ready for review May 14, 2024 01:38

adamreeve requested review from assignUser and raulcd as code owners May 14, 2024 01:38

adamreeve force-pushed the msvc_glib branch from beffceb to 4dc31f8 Compare May 14, 2024 01:39

adamreeve and others added 20 commits May 22, 2024 16:54

Fix for different pkgconfig location on Windows

09e8974

Workflow tidy up

957fa17

Simplify getting vcpkg version from .env

7671e93

Auto update vcpkg.json version string

5373dfd

Change visibility macros to more closer match glib convention

5f2fd61

Only add dllexport/import attributes for MSVC for now

4cda2cc

Move fix for finding gnu link.exe to c_glib_build.sh

36ba0e3

Use meson --pkg-config-path option

19047b2

Workaround for linker error due to space in Arrow install path

e386ece

Fix JNI build in docker failing due to no .env file

f51193c

Whitespace fixes from code review

87ae36c

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Header tidy ups from code review

ddd52a1

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

Remove setting CMAKE_INSTALL_LIBDIR

208e7eb

Generate visibility headers

8561512

Fix accidentally added semicolons in header

af2565c

Add extra COMPILATION definitions

b687257

Enable building other GLib libraries except Gandiva

a4da68a

Add _EXTERN macros as required to allow cross-lib use

bb02be9

Disable zlib

0a76c7f

Disable building Flight and Flight SQL

d4e4563

adamreeve force-pushed the msvc_glib branch from 0542ea1 to d4e4563 Compare May 23, 2024 03:00

github-actions bot added Component: Parquet Component: C++ - Gandiva labels May 23, 2024

kou reviewed May 23, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 23, 2024

Include visibility macros in version.h

39c8278

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41134: [GLib] Support building arrow-glib with MSVC #41599

GH-41134: [GLib] Support building arrow-glib with MSVC #41599

adamreeve commented May 9, 2024 •

edited

github-actions bot commented May 9, 2024

kou left a comment

kou May 10, 2024

kou May 10, 2024

adamreeve May 14, 2024

kou May 15, 2024

kou May 10, 2024

adamreeve May 10, 2024

kou May 10, 2024

adamreeve May 10, 2024 •

edited

adamreeve May 14, 2024

kou May 15, 2024

adamreeve May 16, 2024

adamreeve May 14, 2024

raulcd commented May 14, 2024

github-actions bot commented May 14, 2024

raulcd commented May 14, 2024

github-actions bot commented May 14, 2024

adamreeve commented May 23, 2024

kou May 23, 2024

adamreeve May 23, 2024



		def write_header(output_file: TextIOBase, library: str):
		output_file.write(f"""#pragma once

GH-41134: [GLib] Support building arrow-glib with MSVC #41599

Are you sure you want to change the base?

GH-41134: [GLib] Support building arrow-glib with MSVC #41599

Conversation

adamreeve commented May 9, 2024 • edited

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 9, 2024

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreeve May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulcd commented May 14, 2024

github-actions bot commented May 14, 2024

raulcd commented May 14, 2024

github-actions bot commented May 14, 2024

adamreeve commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreeve commented May 9, 2024 •

edited

adamreeve May 10, 2024 •

edited