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

Update googletest from 1.8.1 to 1.10.0 #6808

Closed
wants to merge 4 commits into from

Conversation

adamretter
Copy link
Collaborator

No description provided.

@pdillinger
Copy link
Contributor

Because you want some new features? (just curious)

@adamretter
Copy link
Collaborator Author

@pdillinger I am working on some tests for improving the WriteBatch and WriteBatchWithIndex, I wanted to write some parameterized tests. I was getting compilation errors for the parameterized test syntax with 1.8.1, updating to 1.10.0 fixed this.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pdillinger
Copy link
Contributor

CircleCI failure is just #6232

@pdillinger
Copy link
Contributor

Actually, this is not so easy because our Facebook buck build uses a gtest 1.8.x version not in the RocksDB repository. What do you think?

@adamretter
Copy link
Collaborator Author

adamretter commented May 9, 2020

@pdillinger Unfortunately I don't have access to any internal FB systems.

I tried to understand how the Buck build might work, but the public instructions for Buck seem to vary from how the RocksDB project is set-out. On a chance I ran buckifier/buckify_rocksdb.py but the generated TARGETS file didn't help me. Also within that file is the text:

This file is a Facebook-specific integration for buck builds, so can
only be validated by Facebook employees.

Without access to internal systems I am not sure I can help with the Buck stuff, unless you can point me at some docs or something?

@adamretter
Copy link
Collaborator Author

@pdillinger Any updates please?

pdillinger added a commit to pdillinger/rocksdb that referenced this pull request May 19, 2020
Summary: ... so that we have freedom to upgrade it (see facebook#6808).

As a side benefit, gtest will no longer be linked into main library in
buck build.

Test Plan: fb internal build & link
facebook-github-bot pushed a commit that referenced this pull request May 20, 2020
Summary:
... so that we have freedom to upgrade it (see #6808).

As a side benefit, gtest will no longer be linked into main library in
buck build.
Pull Request resolved: #6858

Test Plan: fb internal build & link

Reviewed By: riversand963

Differential Revision: D21652061

Pulled By: pdillinger

fbshipit-source-id: 6018104af944debde576b5beda6c134e737acedb
@facebook-github-bot
Copy link
Contributor

@adamretter has updated the pull request. Re-import the pull request

@adamretter
Copy link
Collaborator Author

@pdillinger Thanks, I have now updated this to incorporate your PR #6858

@pdillinger
Copy link
Contributor

I can't add commits. Please run

git diff -U0 --diff-filter=M b559dba817bdb2e23d6576acfae3ca3378ebb29d | clang-format-diff.py -i -p 1

to fix formatting (other than gtest)

@adamretter
Copy link
Collaborator Author

@pdillinger Thanks. I have done that now.

@facebook-github-bot
Copy link
Contributor

@adamretter has updated the pull request. Re-import the pull request

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pdillinger
Copy link
Contributor

TARGETS says 1.10.1 instead of 1.10.0. Need to re-run python buckifier/buckify_rocksdb.py?

@facebook-github-bot
Copy link
Contributor

@adamretter has updated the pull request. Re-import the pull request

@adamretter
Copy link
Collaborator Author

@pdillinger Doh! Okay I just fixed that TARGETS one and force-pushed

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@pdillinger merged this pull request in 8d87e9c.

siying added a commit to siying/rocksdb that referenced this pull request Jun 3, 2020
This reverts commit 8d87e9c.

Based on offline discussions, it's too early to upgrade to gtest 1.10, as it prevents some developers from using an older version of gtest to integrate to some other systems. Revert it for now.
facebook-github-bot pushed a commit that referenced this pull request Jun 3, 2020
Summary:
This reverts commit 8d87e9c.

Based on offline discussions, it's too early to upgrade to gtest 1.10, as it prevents some developers from using an older version of gtest to integrate to some other systems. Revert it for now.
Pull Request resolved: #6923

Reviewed By: pdillinger

Differential Revision: D21864799

fbshipit-source-id: d0726b1ff649fc911b9378f1763316200bd363fc
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.

3 participants