Skip to content

GH-34813: [C++] Improve GoogleTest detection - #34920

Merged
kou merged 1 commit into
apache:mainfrom
kou:cpp-gtest-fallback
Apr 6, 2023
Merged

GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom
kou:cpp-gtest-fallback

Conversation

@kou

@koukou commented Apr 6, 2023•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

#34765 approach has some corner cases. They causes CI failures.

What changes are included in this PR?

If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

@github-actions

github-actionsBot commented Apr 6, 2023

Copy link
Copy Markdown

@github-actions

github-actionsBot commented Apr 6, 2023

Copy link
Copy Markdown

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

@kou
kouforce-pushed the cpp-gtest-fallback branch 2 times, most recently from d8ccf7e to 199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed the cpp-gtest-fallback branch from 199647d to a48266eCompareApril 6, 2023 03:33
@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-alpine-linux-cpp

@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed the cpp-gtest-fallback branch from a48266e to 536af6aCompareApril 6, 2023 05:39
@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-alpine-linux-cpp

@kou
kouforce-pushed the cpp-gtest-fallback branch from 536af6a to a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed the cpp-gtest-fallback branch 2 times, most recently from 0b9cf98 to 3440365CompareApril 6, 2023 06:13
@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-alpine-linux-cpp

@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed the cpp-gtest-fallback branch from 3440365 to 3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed the cpp-gtest-fallback branch from 3de38f0 to 3815328CompareApril 6, 2023 06:52
@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit test-alpine-linux-cpp

@github-actions

github-actionsBot commented Apr 6, 2023

Copy link
Copy Markdown

Revision: 3815328

Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147

TaskStatus
test-alpine-linux-cppGithub Actions

@jorisvandenbossche

jorisvandenbossche commented Apr 6, 2023

Copy link
Copy Markdown
Member

Confirming that the latest version of the PR seems to work for me (with a gtest from conda):

...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)

assignUser
assignUser reviewed Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}

@assignUserassignUserApr 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.

I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)

@koukouApr 6, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 6, 2023
@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

Confirming that the latest version of the PR seems to work for me (with a gtest from conda):

Thanks for confirming this!

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)

Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.

@kou

kou commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

+1

@kou
kou merged commit f7644ae into apache:mainApr 6, 2023
@kou
kou deleted the cpp-gtest-fallback branch April 6, 2023 23:52
@ursabot

ursabot commented Apr 8, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f7644ae8 ec2-t3-xlarge-us-east-2
[Finished] f7644ae8 test-mac-arm
[Finished] f7644ae8 ursa-i9-9960x
[Finished] f7644ae8 ursa-thinkcentre-m75q
[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2
[Failed] 6f3d96ec test-mac-arm
[Finished] 6f3d96ec ursa-i9-9960x
[Failed] 6f3d96ec 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

@zfoobar

zfoobar commented Apr 8, 2023•
edited
Loading

Copy link
Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).

The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.

It's back to the old behavior, build fails with the symbols it can't resolve.

@kou

kou commented Apr 9, 2023

Copy link
Copy Markdown
MemberAuthor

Could you open a new issue with how to reproduce and full error log?

At least I can't reproduce it with macOS/arm64/Homebrew(not conda).

@zfoobar

zfoobar commented Apr 9, 2023 via email

Copy link
Copy Markdown

@kou

kou commented Apr 9, 2023

Copy link
Copy Markdown
MemberAuthor

cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees

No one assigned

Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.

5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
GH-34813: [C++] Improve GoogleTest detection by kou · Pull Request #34920 · apache/arrow · GitHub
Skip to content
GH-34813: [C++] Improve GoogleTest detection - #34920
Merged
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Apr 6, 2023
Merged
GH-34813: [C++] Improve GoogleTest detection#34920
kou merged 1 commit into
apache:mainfrom 
kou:cpp-gtest-fallback
Conversation
@kou
@koukou
commented
Apr 6, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
#34765 approach has some corner cases. They causes CI failures.
What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34813has been automatically assigned in GitHub to PR creator.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
d8ccf7e to
199647dCompareApril 6, 2023 03:30
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
199647d to
a48266eCompareApril 6, 2023 03:33
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
a48266e to
536af6aCompareApril 6, 2023 05:39
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
536af6a to
a585e35CompareApril 6, 2023 05:39
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
2 times, most recently
from
0b9cf98 to
3440365CompareApril 6, 2023 06:13
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions

This comment was marked as outdated.

Sign in to view
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3440365 to
3de38f0CompareApril 6, 2023 06:16
@kou
apacheGH-34813: [C++] Improve GoogleTest detection
3815328
If incompatible GoogleTest is detected, we can fallback to bundled
GoogleTest automatically.
@kou
kouforce-pushed
the
cpp-gtest-fallback
branch
from
3de38f0 to
3815328CompareApril 6, 2023 06:52
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit test-alpine-linux-cpp
@github-actions
github-actionsBot
commented
Apr 6, 2023

Copy link


Copy Markdown

Revision: 3815328
Submitted crossbow builds: ursacomputing/crossbow @ actions-e991323147
TaskStatus
test-alpine-linux-cppGithub Actions
@jorisvandenbossche
jorisvandenbossche
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
...
-- Found GTest: /home/joris/miniconda3/envs/arrow-dev/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.13.0", minimum required is "1.10.0") ...

On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
assignUser
assignUser
reviewed
Apr 6, 2023
View reviewed changes
Comment threadcpp/CMakeLists.txt

set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers GTest::gtest_main GTest::gtest
GTest::gmock)
set(ARROW_TEST_LINK_TOOLCHAIN arrow::flatbuffers ${ARROW_GTEST_GTEST_MAIN}
@assignUserassignUserApr 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this is a regression as using variables instead of targets is an anti-pattern but I currently don't see a cleaner approach either.
I think we should rethink the entire dependency management once we have bumped the cmake version to 3.16 as we will have much better tools available. (this is of course out of scope for this PR)
@koukouApr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use add_library(ALIAS) with CMake 3.11: https://cmake.org/cmake/help/latest/command/add_library.html#alias-libraries
@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Apr 6, 2023
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
Confirming that the latest version of the PR seems to work for me (with a gtest from conda):
Thanks for confirming this!
On a previous version I got an error like "CMake Error: The source directory "SOURCES/CMakeFiles/CMakeTmp" does not exist" from FindGTestAlt.cmake:53 (try_compile)
Yes. I needed to write a code to work with newer CMake and older CMake. On a previous version this implementation worked only with newer CMake.
@kou
kou
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
+1
@kou
kou
merged commit f7644ae
into
apache:mainApr 6, 2023
@kou
kou
deleted the

cpp-gtest-fallback

branch
April 6, 2023 23:52
@ursabot
ursabot
commented
Apr 8, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = 6f3d96e and contender = f7644ae. f7644ae 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 ⬇️3.09% ⬆️1.16%] test-mac-arm

[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x

[Failed ⬇️1.14% ⬆️0.98%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] f7644ae8 ec2-t3-xlarge-us-east-2

[Finished] f7644ae8 test-mac-arm

[Finished] f7644ae8 ursa-i9-9960x

[Finished] f7644ae8 ursa-thinkcentre-m75q

[Finished] 6f3d96ec ec2-t3-xlarge-us-east-2

[Failed] 6f3d96ec test-mac-arm

[Finished] 6f3d96ec ursa-i9-9960x

[Failed] 6f3d96ec 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
@zfoobar
zfoobar
commented
Apr 8, 2023•

edited

Loading

Copy link


Copy Markdown

This is a regression - build fails for me (macos/arm64/conda).
The only reason I could get it building is because I've been tracking this issue and knew to supply the proper BUNDLED flag. At least before the cmake module prompted you to put the flag in place if it detected the installed binaries weren't c++17.
It's back to the old behavior, build fails with the symbols it can't resolve.
@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
Could you open a new issue with how to reproduce and full error log?
At least I can't reproduce it with macOS/arm64/Homebrew(not conda).
@zfoobar
zfoobar
commented
Apr 9, 2023
via email 

Copy link


Copy Markdown

@kou
kou
commented
Apr 9, 2023

Copy link


Copy Markdown

MemberAuthor
cmake --debug-trycompile ... will help us: https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-debug-trycompile
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@kou@liujiacheng777
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
0e37cef
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@kou@ArgusLi
apacheGH-34813: [C++] Improve GoogleTest detection (apache#34920)
a58123d
### Rationale for this change
apache#34765 approach has some corner cases. They causes CI failures.
### What changes are included in this PR?
If incompatible GoogleTest is detected, we can fallback to bundled GoogleTest automatically.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: apache#34813
Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@assignUserassignUserassignUser left review comments
@wjones127wjones127Awaiting requested review from wjones127
@lidavidmlidavidmAwaiting requested review from lidavidmlidavidm is a code owner
@raulcdraulcdAwaiting requested review from raulcdraulcd is a code owner

Assignees


No one assigned


Labels

awaiting changesAwaiting changesComponent: C++Component: FlightRPCComponent: Parquet

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Add an auto fallback solution when system Gtest is compiled with a C++ standard lower than 17.


5 participants

@kou@jorisvandenbossche@ursabot@zfoobar@assignUser