-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
GH-23221: [C++] Add support for building with Emscripten #37821
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
@kou here are the cmake changes for building libarrow in emscripten. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry. My review isn't completed yet. I'll continue later.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(My first review is completed.)
cpp/cmake_modules/Emscripten/Platform/EmscriptenOverrides.cmake
Outdated
Show resolved
Hide resolved
cpp/cmake_modules/Emscripten/Platform/EmscriptenOverrides.cmake
Outdated
Show resolved
Hide resolved
cpp/cmake_modules/Emscripten/Platform/EmscriptenOverrides.cmake
Outdated
Show resolved
Hide resolved
cpp/cmake_modules/Emscripten/Platform/EmscriptenOverrides.cmake
Outdated
Show resolved
Hide resolved
cpp/cmake_modules/Emscripten/Platform/EmscriptenOverrides.cmake
Outdated
Show resolved
Hide resolved
Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>
@github-actions crossbow submit test-ubuntu-22.04-cpp-emscripten |
Revision: d3bc3b4 Submitted crossbow builds: ursacomputing/crossbow @ actions-b7535ddb15
|
@github-actions crossbow submit -g cpp |
Revision: d3bc3b4 Submitted crossbow builds: ursacomputing/crossbow @ actions-fd564c7b19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thanks for your reviews @kou and thanks @joemarshall for all the hard work!! |
Brilliant, thanks everyone |
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 1a1d2c8. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them. |
@github-actions crossbow submit test-build-vcpkg-win |
I am trying to understand if this PR was the one causing the |
Revision: 1307910 Submitted crossbow builds: ursacomputing/crossbow @ actions-5c4dbcd949
|
apt-get clean && \ | ||
rm -rf /var/lib/apt/lists* | ||
|
||
# install emscripten using EMSDK | ||
ARG emscripten_version="3.1.45" | ||
RUN cd ~ && git clone https://github.com/emscripten-core/emsdk.git && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, two things:
- it would have been better for resource usage to make a separate dockerfile for this, perhaps
- could this be a script to avoid having tons of hand-coded bash code in dockerfiles?
@@ -151,6 +162,9 @@ RUN if [ "${gcc_version}" = "" ]; then \ | |||
update-alternatives --set c++ /usr/bin/g++; \ | |||
fi | |||
|
|||
# make sure zlib is cached in the EMSDK folder | |||
RUN source ~/emsdk/emsdk_env.sh && embuilder --pic build zlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this isn't together with the install step above?
…41178) ### Rationale for this change #37821 changed to use `add_test()` usage from old style to new style: 1a1d2c8?diff=unified&w=1#diff-1ce47eec54afaee769086e1a720c5ed65bc347cd8fc60a233de67fd895dda329L763-R764 MSVC generators multi-config generators. With old style, all tests are run without specifying `--build-config` explicitly. With new style, we need to specify `--build-config` explicitly. See also: https://cmake.org/cmake/help/latest/command/add_test.html ### What changes are included in this PR? Specify `--build-config` explicitly. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #41169 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ndows (apache#41178) ### Rationale for this change apache#37821 changed to use `add_test()` usage from old style to new style: apache@1a1d2c8?diff=unified&w=1#diff-1ce47eec54afaee769086e1a720c5ed65bc347cd8fc60a233de67fd895dda329L763-R764 MSVC generators multi-config generators. With old style, all tests are run without specifying `--build-config` explicitly. With new style, we need to specify `--build-config` explicitly. See also: https://cmake.org/cmake/help/latest/command/add_test.html ### What changes are included in this PR? Specify `--build-config` explicitly. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#41169 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…41178) ### Rationale for this change #37821 changed to use `add_test()` usage from old style to new style: 1a1d2c8?diff=unified&w=1#diff-1ce47eec54afaee769086e1a720c5ed65bc347cd8fc60a233de67fd895dda329L763-R764 MSVC generators multi-config generators. With old style, all tests are run without specifying `--build-config` explicitly. With new style, we need to specify `--build-config` explicitly. See also: https://cmake.org/cmake/help/latest/command/add_test.html ### What changes are included in this PR? Specify `--build-config` explicitly. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: #41169 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…e#37821) Split from apache#37696 This is just the cmake changes to enable building on emscripten. Changes are: 1) Support for target system "emscripten" 2) Cmake preset for building libarrow ` ninja-release-python-emscripten` (same as `ninja-release-python`, but with emscripten support) 3) Override file for cmake on emscripten, to set various build parameters that need setting to make it build there. 4) Changes in pyarrow cmake so it works if you are building libarrow as shared library, and also an option to enable the cmake file there to just dump the current arrow configuration, which is useful for cross-compile builds. * Closes: apache#23221 Lead-authored-by: Joe Marshall <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ndows (apache#41178) ### Rationale for this change apache#37821 changed to use `add_test()` usage from old style to new style: apache@1a1d2c8?diff=unified&w=1#diff-1ce47eec54afaee769086e1a720c5ed65bc347cd8fc60a233de67fd895dda329L763-R764 MSVC generators multi-config generators. With old style, all tests are run without specifying `--build-config` explicitly. With new style, we need to specify `--build-config` explicitly. See also: https://cmake.org/cmake/help/latest/command/add_test.html ### What changes are included in this PR? Specify `--build-config` explicitly. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#41169 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…e#37821) Split from apache#37696 This is just the cmake changes to enable building on emscripten. Changes are: 1) Support for target system "emscripten" 2) Cmake preset for building libarrow ` ninja-release-python-emscripten` (same as `ninja-release-python`, but with emscripten support) 3) Override file for cmake on emscripten, to set various build parameters that need setting to make it build there. 4) Changes in pyarrow cmake so it works if you are building libarrow as shared library, and also an option to enable the cmake file there to just dump the current arrow configuration, which is useful for cross-compile builds. * Closes: apache#23221 Lead-authored-by: Joe Marshall <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…e#37821) Split from apache#37696 This is just the cmake changes to enable building on emscripten. Changes are: 1) Support for target system "emscripten" 2) Cmake preset for building libarrow ` ninja-release-python-emscripten` (same as `ninja-release-python`, but with emscripten support) 3) Override file for cmake on emscripten, to set various build parameters that need setting to make it build there. 4) Changes in pyarrow cmake so it works if you are building libarrow as shared library, and also an option to enable the cmake file there to just dump the current arrow configuration, which is useful for cross-compile builds. * Closes: apache#23221 Lead-authored-by: Joe Marshall <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…e#37821) Split from apache#37696 This is just the cmake changes to enable building on emscripten. Changes are: 1) Support for target system "emscripten" 2) Cmake preset for building libarrow ` ninja-release-python-emscripten` (same as `ninja-release-python`, but with emscripten support) 3) Override file for cmake on emscripten, to set various build parameters that need setting to make it build there. 4) Changes in pyarrow cmake so it works if you are building libarrow as shared library, and also an option to enable the cmake file there to just dump the current arrow configuration, which is useful for cross-compile builds. * Closes: apache#23221 Lead-authored-by: Joe Marshall <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…e#37821) Split from apache#37696 This is just the cmake changes to enable building on emscripten. Changes are: 1) Support for target system "emscripten" 2) Cmake preset for building libarrow ` ninja-release-python-emscripten` (same as `ninja-release-python`, but with emscripten support) 3) Override file for cmake on emscripten, to set various build parameters that need setting to make it build there. 4) Changes in pyarrow cmake so it works if you are building libarrow as shared library, and also an option to enable the cmake file there to just dump the current arrow configuration, which is useful for cross-compile builds. * Closes: apache#23221 Lead-authored-by: Joe Marshall <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Joris Van den Bossche <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ndows (apache#41178) ### Rationale for this change apache#37821 changed to use `add_test()` usage from old style to new style: apache@1a1d2c8?diff=unified&w=1#diff-1ce47eec54afaee769086e1a720c5ed65bc347cd8fc60a233de67fd895dda329L763-R764 MSVC generators multi-config generators. With old style, all tests are run without specifying `--build-config` explicitly. With new style, we need to specify `--build-config` explicitly. See also: https://cmake.org/cmake/help/latest/command/add_test.html ### What changes are included in this PR? Specify `--build-config` explicitly. ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#41169 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Split from #37696
This is just the cmake changes to enable building on emscripten.
Changes are:
ninja-release-python-emscripten
(same asninja-release-python
, but with emscripten support)