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.

Sign up for GitHub

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

Jump to bottom

Relax warnings as errors so we can build on Clang 11 #123

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Relax warnings as errors so we can build on Clang 11 #123

merged 1 commit into from
Sep 26, 2019

Conversation

nrc
Copy link

@nrc nrc commented Sep 25, 2019

This prevents TiKV building on MacOS.

PTAL @yiwu-arbug @Little-Wallace

@nrc nrc mentioned this pull request Sep 25, 2019
Merged
@Connor1996
Copy link
Member

Busy working on other things, sorry for not fixing it in time. Thanks!

Connor1996
Connor1996 approved these changes Sep 25, 2019
View reviewed changes
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@yiwu-arbug
Copy link
Collaborator

@nrc Adding these flags to corresponding rust-rocksdb build.rs scripts would avoid the need to modify rocksdb. Do you mind sending a PR there instead?

@nrc
Copy link
Author

nrc commented Sep 25, 2019

@yiwu-arbug I don't know how to do that - I tried adding cfg.cxxflag("-Wno-error=shadow"); to build_rocksdb, but I still got the new errors when building.

@aknuds1
Copy link

aknuds1 commented Sep 25, 2019

Thanks, presuming this fixes TiKV build on OS X.

@yiwu-arbug
Copy link
Collaborator

@nrc have you try adding cxxflag("-Wno-error=shadow") to both librocksdb/build.rs and librocksdb/libtitan/build.rs?

@yiwu-arbug
Copy link
Collaborator

@nrc I'm not able to test since I cannot reproduce the error on my mac (also with clang 11). Don't know why :(

@nrc
Copy link
Author

nrc commented Sep 25, 2019

have you try adding cxxflag("-Wno-error=shadow") to both librocksdb/build.rs and librocksdb/libtitan/build.rs?

I have, and it still fails - from the logs, the flags are being passed to cmake properly, but I think the flags from CMakeLists.txt override them? I'm not sure about either Clang's or CMake's behaviour tbh.

Thinking about it I think it might be better to fix it here (and in Titan) rather than rust-rocksdb, since this will be a problem for people building these projects outside of rust-rocksdb too.

@nrc
Copy link
Author

nrc commented Sep 25, 2019

Seems like the unknown error is causing problems with older Clang on CI

@Hoverbear
Copy link

Seems related to tikv/rust-rocksdb#295

@yiwu-arbug
Copy link
Collaborator

have you try adding cxxflag("-Wno-error=shadow") to both librocksdb/build.rs and librocksdb/libtitan/build.rs?

I have, and it still fails - from the logs, the flags are being passed to cmake properly, but I think the flags from CMakeLists.txt override them? I'm not sure about either Clang's or CMake's behaviour tbh.

Thinking about it I think it might be better to fix it here (and in Titan) rather than rust-rocksdb, since this will be a problem for people building these projects outside of rust-rocksdb too.

just tried and seems it's because rocksdb's CMakeList.txt sets -Wshadow, which comes later in the compile command and override what's set from build.rs. I'm okay to land this. Do you mind fix the build? Seems CI is not happy with -Wno-error=defaulted-function-deleted.

@nrc
Relax warnings as errors so we can build on Clang 11
9507d3b 
Signed-off-by: Nick Cameron <nrc@ncameron.org>
@Hoverbear Hoverbear mentioned this pull request Sep 26, 2019
Closed
@yiwu-arbug yiwu-arbug merged commit 9c5b744 into tikv:tikv-3.0 Sep 26, 2019
yiwu-arbug pushed a commit to tikv/titan that referenced this pull request Sep 26, 2019
@nrc
Relax warnings as errors so we can build on Clang 11 (#90)
974e5c6 
This prevents TiKV building on MacOS. See also tikv/rocksdb#123

Signed-off-by: Nick Cameron <nrc@ncameron.org>
yiwu-arbug pushed a commit that referenced this pull request Sep 26, 2019
@nrc
Relax warnings as errors so we can build on Clang 11 (#123)
710819d 
Signed-off-by: Nick Cameron <nrc@ncameron.org>
yiwu-arbug pushed a commit that referenced this pull request Oct 30, 2019
Cherry-pick compile error fixes (#129)
d9ac746 
* Revert "Relax warnings as errors so we can build on Clang 11 (#123)"

This reverts commit 710819d.

* Rename InternalDBStatsType enum names (facebook#5779)

Summary:
When building with clang 9, warning is reported for InternalDBStatsType type names shadowed the one for statistics. Rename them.
Pull Request resolved: facebook#5779

Test Plan: Build with clang 9 and see it passes.

Differential Revision: D17239378

fbshipit-source-id: af28fb42066c738cd1b841f9fe21ab4671dafd18

* Fix compiler error by deleting GetContext default ctor (facebook#5685)

Summary:
When updating compiler version for MyRocks I'm seeing this error with rocksdb:

```
ome/yzha/mysql/mysql-fork2/rocksdb/table/get_context.h:91:3: error: explicitly defaulted default constructor is implicitly deleted
      [-Werror,-Wdefaulted-function-deleted]
  GetContext() = default;
  ^
/home/yzha/mysql/mysql-fork2/rocksdb/table/get_context.h:166:18: note: default constructor of 'GetContext' is implicitly deleted because field
      'tracing_get_id_' of const-qualified type 'const uint64_t' (aka 'const unsigned long') would not be initialized
  const uint64_t tracing_get_id_;
                 ^
```

The error itself is rather self explanatory and makes sense.

Given that no one seems to be using the default ctor (they shouldn't, anyway), I'm deleting it.
Pull Request resolved: facebook#5685

Differential Revision: D16747712

Pulled By: yizhang82

fbshipit-source-id: 95c0acb958a1ed41154c0047d2e6fce7644de53f

* Fix a compile error (facebook#5864)

Summary:
```
tools/block_cache_analyzer/block_cache_trace_analyzer.cc:653:48: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'std::__1::linear_congruential_engine<unsigned int, 48271, 0, 2147483647>::result_type' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
  std::default_random_engine rand_engine(env_->NowMicros());
```
Pull Request resolved: facebook#5864

Differential Revision: D17668962

fbshipit-source-id: e08fa58b2a78a8dd8b334862b5714208f696b8ab
@sre-bot sre-bot mentioned this pull request Nov 21, 2019
Closed
Connor1996 pushed a commit to Connor1996/rocksdb that referenced this pull request Nov 21, 2019
@Connor1996
Cherry-pick compile error fixes (tikv#129)
d773fad 
* Revert "Relax warnings as errors so we can build on Clang 11 (tikv#123)"

This reverts commit 710819d.

* Rename InternalDBStatsType enum names (facebook#5779)

Summary:
When building with clang 9, warning is reported for InternalDBStatsType type names shadowed the one for statistics. Rename them.
Pull Request resolved: facebook#5779

Test Plan: Build with clang 9 and see it passes.

Differential Revision: D17239378

fbshipit-source-id: af28fb42066c738cd1b841f9fe21ab4671dafd18

* Fix compiler error by deleting GetContext default ctor (facebook#5685)

Summary:
When updating compiler version for MyRocks I'm seeing this error with rocksdb:

```
ome/yzha/mysql/mysql-fork2/rocksdb/table/get_context.h:91:3: error: explicitly defaulted default constructor is implicitly deleted
      [-Werror,-Wdefaulted-function-deleted]
  GetContext() = default;
  ^
/home/yzha/mysql/mysql-fork2/rocksdb/table/get_context.h:166:18: note: default constructor of 'GetContext' is implicitly deleted because field
      'tracing_get_id_' of const-qualified type 'const uint64_t' (aka 'const unsigned long') would not be initialized
  const uint64_t tracing_get_id_;
                 ^
```

The error itself is rather self explanatory and makes sense.

Given that no one seems to be using the default ctor (they shouldn't, anyway), I'm deleting it.
Pull Request resolved: facebook#5685

Differential Revision: D16747712

Pulled By: yizhang82

fbshipit-source-id: 95c0acb958a1ed41154c0047d2e6fce7644de53f

* Fix a compile error (facebook#5864)

Summary:
```
tools/block_cache_analyzer/block_cache_trace_analyzer.cc:653:48: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long long') to 'std::__1::linear_congruential_engine<unsigned int, 48271, 0, 2147483647>::result_type' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
  std::default_random_engine rand_engine(env_->NowMicros());
```
Pull Request resolved: facebook#5864

Differential Revision: D17668962

fbshipit-source-id: e08fa58b2a78a8dd8b334862b5714208f696b8ab
@tabokie tabokie mentioned this pull request May 9, 2022
Closed
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yiwu-arbug yiwu-arbug yiwu-arbug approved these changes

@Connor1996 Connor1996 Connor1996 approved these changes

@Little-Wallace Little-Wallace Awaiting requested review from Little-Wallace

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nrc @Connor1996 @yiwu-arbug @aknuds1 @Hoverbear