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.

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

ARROW-17433: [CI][C++] Use Visual Studio 2019 on AppVeyor #13903

Merged
merged 4 commits into from Aug 25, 2022

Conversation

kou
Copy link
Member

@kou kou commented Aug 17, 2022

We can use /external:I for Boost to suppress warnings from Boost
with Visual Studio 2019 or later.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@kou
Copy link
Member Author

kou commented Aug 17, 2022

@pitrou Can we drop support for Visual Studio 2017? Visual Studio 2017 reached EOL at 2022-04-12: https://docs.microsoft.com/en-us/lifecycle/products/visual-studio-2017

Listing Start Date Mainstream End Date Extended End Date
Visual Studio 2017 Mar 7, 2017 Apr 12, 2022 Apr 13, 2027

If we can drop support for Visual Studio 2017, we can fix this problem.

This problem is caused by the change boostorg/process#250 (comment) in Boost 1.80. Our build configuration on AppVeyor marks a warning as an error. With Boost 1.80, a warning in Boost causes the CI failure.

We can solve this by marking Boost as an external (header) library. We can use /external:I (equivalent to -isystem in GCC) instead of /I to specify include directory for Boost to mark Boost as an external (header) library. If we use /external:I, warnings in Boost aren't reported.

CMake 3.22 https://cmake.org/cmake/help/latest/release/3.22.html added support for /external:I with Ninja generator:

The Ninja and NMake Makefiles generators now use the MSVC -external:I flag for system includes. This became available as of VS 16.10 (toolchain version 14.29.30037).

But it requires Visual Studio 2019 or later because the /external:I flag is an experimental feature with Visual Studio 2017.

If we can drop support for Visual Studio 2017, we can remove all CI jobs on AppVeyor because GitHub Actions provides Visual Studio 2019 or later.

@pitrou
Copy link
Member

pitrou commented Aug 17, 2022

@kou This would probably warn on later MSVC versions also.

@pitrou
Copy link
Member

pitrou commented Aug 17, 2022

As for not supporting VS2017, I think this is better asked on the ML.

@kou
Copy link
Member Author

kou commented Aug 17, 2022

This would probably warn on later MSVC versions also.

You mean that warnings from Boost? If so, they aren't reported with /external:I.

As for not supporting VS2017, I think this is better asked on the ML.

OK. I'll send an e-mail.

@kou
Copy link
Member Author

kou commented Aug 17, 2022

@pitrou
Copy link
Member

pitrou commented Aug 18, 2022

@kou Is the AppVeyor failure expected here?

@kou
Copy link
Member Author

kou commented Aug 19, 2022

No. It's unexpected.

I just tried to use Visual Studio 2019 on AppVeyer but it shows that we need to update our AppVeyor related scripts. So I stopped the work.

And I found that we don't need CMake changes in this pull request to mark Boost as an external library. CMake marks IMPORTED libraries as external libraries by default: https://cmake.org/cmake/help/latest/prop_tgt/NO_SYSTEM_FROM_IMPORTED.html

@pitrou
Copy link
Member

pitrou commented Aug 23, 2022

@kou What is needed to move this forward? Ideally we'd like to unblock the AppVeyor CI.

@kou
Copy link
Member Author

kou commented Aug 23, 2022

I'm waiting for comments from others on the mailing list thread.
I'll migrate Conda + Visual Studio CI job on AppVeyor to GitHub Actions (.github/workflows/cpp.yml and .github/workflows/python.yml) if nobody objects dropping support for Visual Studio 2017 by tomorrow (1 week from my e-mail).

I think that we can remove the "JOB=Building_Debug, GENERATOR=Ninja" on AppVeyor job because the "AMD64 Windows 2019 C++17" on GitHub Actions job almost covers the case.

@pitrou
Copy link
Member

pitrou commented Aug 23, 2022

I'll migrate Conda + Visual Studio CI job on AppVeyor to GitHub Actions

It may be nice to keep a job on AppVeyor because we have long queues on GHA already.

@kou
Copy link
Member Author

kou commented Aug 23, 2022

Hmm, I think that maintaining AppVeyor related scripts is high cost for us... but OK. I'll keep a job on AppVeyor.

BTW, do you agree with removing the "JOB=Building_Debug, GENERATOR=Ninja" job?

@pitrou
Copy link
Member

pitrou commented Aug 23, 2022

Sounds ok (even though the GHA build is much slower).

@kou kou force-pushed the cpp-boost-system branch 3 times, most recently from bbaeaa0 to fc13e77 Compare August 24, 2022 00:53
@kou kou force-pushed the cpp-boost-system branch 2 times, most recently from a601656 to 8a7b340 Compare August 24, 2022 01:30
@kou kou force-pushed the cpp-boost-system branch 3 times, most recently from 3b9a26b to 59dec29 Compare August 24, 2022 02:46
@kou kou changed the title ARROW-17433: [C++] Make Boost's include directory a system include directory ARROW-17433: [CI][C++] Use Visual Studio 2019 on AppVeyor Aug 24, 2022
@kou kou force-pushed the cpp-boost-system branch 3 times, most recently from da16a5f to 4817b3d Compare August 24, 2022 04:32
@kou
Copy link
Member Author

kou commented Aug 24, 2022

@pitrou Could you fix the following build error? (You can push to this branch.)

https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/44560863

FAILED: src/gandiva/CMakeFiles/gandiva_shared.dir/Unity/unity_3_cxx.cxx.obj 
C:\Miniconda38-x64\Scripts\clcache.exe  /nologo /TP -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_MIMALLOC -DARROW_WITH_RE2 -DARROW_WITH_UTF8PROC -DAWS_CAL_USE_IMPORT_EXPORT -DAWS_CHECKSUMS_USE_IMPORT_EXPORT -DAWS_COMMON_USE_IMPORT_EXPORT -DAWS_EVENT_STREAM_USE_IMPORT_EXPORT -DAWS_IO_USE_IMPORT_EXPORT -DAWS_SDK_VERSION_MAJOR=1 -DAWS_SDK_VERSION_MINOR=8 -DAWS_SDK_VERSION_PATCH=186 -DAWS_USE_IO_COMPLETION_PORTS -DBOOST_ALL_DYN_LINK -DBOOST_ALL_NO_LIB -DGANDIVA_EXPORTING -DGANDIVA_LLVM_VERSION=14 -DPROTOBUF_USE_DLLS -DUSE_IMPORT_EXPORT -DUSE_IMPORT_EXPORT=1 -DUSE_WINDOWS_DLL_SEMANTICS -D_CRT_SECURE_NO_WARNINGS -D_ENABLE_EXTENDED_ALIGNED_STORAGE -Dgandiva_shared_EXPORTS -IC:\projects\arrow\cpp\build\src -IC:\projects\arrow\cpp\src -IC:\projects\arrow\cpp\src\generated -external:IC:\Miniconda38-x64\envs\arrow\Library\include -external:IC:\projects\arrow\cpp\thirdparty\flatbuffers\include -external:IC:\projects\arrow\cpp\thirdparty\hadoop\include -external:IC:\projects\arrow\cpp\build\mimalloc_ep\src\mimalloc_ep\include\mimalloc-2.0 -external:W0 /DWIN32 /D_WINDOWS  /GR /EHsc /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING   /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4800 /wd4996 /wd4065  /WX /MP /MD /Od /UNDEBUG -std:c++17 /showIncludes /Fosrc\gandiva\CMakeFiles\gandiva_shared.dir\Unity\unity_3_cxx.cxx.obj /Fdsrc\gandiva\CMakeFiles\gandiva_shared.dir\ /FS -c C:\projects\arrow\cpp\build\src\gandiva\CMakeFiles\gandiva_shared.dir\Unity\unity_3_cxx.cxx
C:\projects\arrow\cpp\src\arrow/vendored/datetime/tz.h(1792): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'arrow_vendored::date::zoned_time<std::chrono::duration<__int64,std::milli>,const arrow_vendored::date::time_zone *>'
C:\projects\arrow\cpp\src\arrow/vendored/datetime/tz.h(1792): note: No constructor could take the source type, or constructor overload resolution was ambiguous
C:/projects/arrow/cpp/src/gandiva/gdv_function_stubs.cc(764): note: see reference to function template instantiation 'arrow_vendored::date::zoned_time<std::chrono::duration<__int64,std::milli>,const arrow_vendored::date::time_zone *> arrow_vendored::date::make_zoned<std::chrono::duration<__int64,std::milli>,const arrow_vendored::date::time_zone*>(const std::string &,const arrow_vendored::date::zoned_time<std::chrono::duration<__int64,std::milli>,const arrow_vendored::date::time_zone *> &)' being compiled
[286/351] Building CXX object src\gandiva\CMakeFiles\gandiva_shared.dir\Unity\unity_1_cxx.cxx.obj
ninja: build stopped: subcommand failed.
(arrow) C:\projects\arrow\cpp\build>set lastexitcode=1 

I don't have Conda + Windows environment. So fixing this is difficult to me...

Note: cpp/cmake_modules/CMakeLists.txt change is the same as #13958 (for xsimd 9.0.0). It's for using xsimd 9.0.0 installed by Conda on AppVeyer.

@pitrou
Copy link
Member

pitrou commented Aug 24, 2022

Hmm, I fixed the AppVeyor issue but there are unrelated parquet-arrow-test failures, do you know what's happening?

@pitrou
Copy link
Member

pitrou commented Aug 24, 2022

It looks like a very old revision of the testing submodule is being checked out: https://github.com/apache/arrow/runs/7989578616?check_suite_focus=true#step:2:606

@pitrou
Copy link
Member

pitrou commented Aug 24, 2022

Rebased.

@pitrou
Copy link
Member

pitrou commented Aug 24, 2022

Looks like CI is fixed. I'll let you proceed as you prefer @kou

@kou
Copy link
Member Author

kou commented Aug 24, 2022

Thanks!!!
I'll merge this after #13958 is merged. Because this pull request includes the change in #13958.

@kou
Copy link
Member Author

kou commented Aug 24, 2022

It looks like a very old revision of the testing submodule is being checked out: https://github.com/apache/arrow/runs/7989578616?check_suite_focus=true#step:2:606

It seems that #13781 changed the testing's commit ID.

kou and others added 4 commits August 25, 2022 13:42
We can use /external:I for Boost to suppress warnings from Boost
with Visual Studio 2019 or later.
@kou
Copy link
Member Author

kou commented Aug 25, 2022

+1

@kou kou merged commit 897c186 into apache:master Aug 25, 2022
@kou kou deleted the cpp-boost-system branch August 25, 2022 06:08
@ursabot
Copy link

ursabot commented Aug 25, 2022

Benchmark runs are scheduled for baseline = 04d2403 and contender = 897c186. 897c186 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.24% ⬆️0.03%] test-mac-arm
[Failed ⬇️0.82% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️2.52% ⬆️0.75%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 897c186f ec2-t3-xlarge-us-east-2
[Failed] 897c186f test-mac-arm
[Failed] 897c186f ursa-i9-9960x
[Finished] 897c186f ursa-thinkcentre-m75q
[Finished] 04d24031 ec2-t3-xlarge-us-east-2
[Failed] 04d24031 test-mac-arm
[Failed] 04d24031 ursa-i9-9960x
[Finished] 04d24031 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

anjakefala pushed a commit to anjakefala/arrow that referenced this pull request Aug 31, 2022
We can use /external:I for Boost to suppress warnings from Boost
with Visual Studio 2019 or later.


Lead-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
We can use /external:I for Boost to suppress warnings from Boost
with Visual Studio 2019 or later.


Lead-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
We can use /external:I for Boost to suppress warnings from Boost
with Visual Studio 2019 or later.


Lead-authored-by: Sutou Kouhei <kou@clear-code.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants