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

Dropping some compiler support in 7.0 #9388

Closed
pdillinger opened this issue Jan 14, 2022 · 5 comments
Closed

Dropping some compiler support in 7.0 #9388

pdillinger opened this issue Jan 14, 2022 · 5 comments
Assignees
@pdillinger
Labels
announcement rocksdb-7.0 PRs with breaking API changes that need to land in the next major release, 7.0.

Comments

@pdillinger
Copy link
Contributor

pdillinger commented Jan 14, 2022 •
edited
Loading

Planned minimum compiler support for RocksDB 7.0 (based on "Stop Supporting Older GCC versions" in https://www.facebook.com/groups/rocksdb.dev/):

C++17 standard (from C++11)
GCC 7.x (from 4.8.x)
Visual Studio 2017 (from 2015)
Clang 5 (from 3.3?)

Please let us know in comments here if you have concerns
@pdillinger pdillinger self-assigned this Jan 14, 2022
@pdillinger pdillinger added announcement rocksdb-7.0 PRs with breaking API changes that need to land in the next major release, 7.0. labels Jan 14, 2022
@pdillinger pdillinger pinned this issue Jan 15, 2022
@pdillinger pdillinger changed the title Planned major release: removing some compiler support Removing some compiler support in 7.0 Jan 15, 2022
@pdillinger pdillinger mentioned this issue Jan 15, 2022
Closed
@pdillinger pdillinger unpinned this issue Jan 15, 2022
@pdillinger pdillinger changed the title Removing some compiler support in 7.0 Dropping some compiler support in 7.0 Jan 15, 2022
pdillinger added a commit to pdillinger/rocksdb that referenced this issue Feb 1, 2022
@pdillinger
Require C++17
d69630b 
Summary: Drop support for some old compilers by requiring C++17 standard
(or higher). See facebook#9388

Test Plan: CI config updated

TODO: finish sanitizing CircleCI config
@pdillinger pdillinger mentioned this issue Feb 1, 2022
Closed
@adamretter
Copy link
Collaborator

Hi @pdillinger I see that GCC 5 was the first to offer C++ 17 features, however if I understand your message above, I think you are saying that GCC 7.0.0 will be considered the minimum compiler required for RocksDB? Is that correct?

At the moment for RocksJava we are using the below compilers for the binary releases. I don't see an issue with getting a newer compiler for CentOS 7 through SCL or similar. However, I will need to look around to find something suitable for CentOS Linux x86 (i.e. 32 bit). I will come back to you shortly with information about what I can do to get these updated for C++ 17 support too

  • CentOS 6 x64 = gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
  • CentOS 6 x86 = gcc (GCC) 4.8.2 20140120 (Red Hat 4.8.2-15)
  • Alpine 3 x64 = gcc (Alpine 8.3.0) 8.3.0
  • Alpine 3 x86 = gcc (Alpine 8.3.0) 8.3.0
  • CentOS 7 ppc64le = gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
  • Alpine 3 ppc64le = gcc (Alpine 8.3.0) 8.3.0
  • CentOS 7 arm64 = gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
  • Alpine 3 arm64 = gcc (Alpine 8.3.0) 8.3.0
  • CentOS 7 s390x = gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44)
  • Alpine 3 s390x = gcc (Alpine 8.3.0) 8.3.0
  • Ubuntu 21.04 RISCV = gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0
  • macOS x64 = Apple clang version 13.0.0 (clang-1300.0.29.30)
  • Windows Server 2021 on x64 with Visual Studio 15.

@pdillinger
Copy link
Contributor Author

I see that GCC 5 was the first to offer C++ 17 features

I was working from this: https://gcc.gnu.org/projects/cxx-status.html A good number of features require GCC 7, and at a minimum 'constexpr if' is one I foresee using.

However, I will need to look around to find something suitable ...

Thanks for the update!

@adamretter
Copy link
Collaborator

adamretter commented Feb 4, 2022 •
edited
Loading

@pdillinger For RocksJava I have now upgraded the following:

  • CentOS 6 x86 from gcc 4.8.2 to 8.3.1; I could not find a usable gcc 7 package for CentOS 6 or 7 on x86, but I did find a gcc 8 package.
  • CentOS 7 ppc64le from gcc 4.8.5 to 7.3.1
  • CentOS 7 arm64 from gcc 4.8.5 to 7.3.1
  • Switched from CentOS 7 s390x to Ubuntu 18.04 s390x with gcc 7.5.0. I chose Ubuntu here over CentOS, as for s390x there are older versions of Ubuntu available than CentOS (a.k.a. ClefOS), which means that a Ubuntu 18 for s390x has the oldest glibc support for s390x whilst still having a gcc > 7

I will do the Windows Cloud VM in the next day or so...

@adamretter adamretter mentioned this issue Feb 4, 2022
Closed
facebook-github-bot pushed a commit that referenced this issue Feb 5, 2022
@pdillinger @facebook-github-bot
Require C++17 (#9481)
fd3e0f4 
Summary:
Drop support for some old compilers by requiring C++17 standard
(or higher). See #9388

First modification based on this is to remove some conditional compilation in slice.h (also
better for ODR)

Also in this PR:
* Fix some Makefile formatting that seems to affect ASSERT_STATUS_CHECKED config in
some cases
* Add c_test to NON_PARALLEL_TEST in Makefile
* Fix a clang-analyze reported "potential leak" in lru_cache_test
* Better "compatibility" definition of DEFINE_uint32 for old versions of gflags
* Fix a linking problem with shared libraries in Makefile (`./random_test: error while loading shared libraries: librocksdb.so.6.29: cannot open shared object file: No such file or directory`)
* Always set ROCKSDB_SUPPORT_THREAD_LOCAL and use thread_local (from C++11)
  * TODO in later PR: clean up that obsolete flag
* Fix a cosmetic typo in c.h (#9488)

Pull Request resolved: #9481

Test Plan:
CircleCI config substantially updated.

* Upgrade to latest Ubuntu images for each release
* Generally prefer Ubuntu 20, but keep a couple Ubuntu 16 builds with oldest supported
compilers, to ensure compatibility
* Remove .circleci/cat_ignore_eagain except for Ubuntu 16 builds, because this is to work
around a kernel bug that should not affect anything but Ubuntu 16.
* Remove designated gcc-9 build, because the default linux build now uses GCC 9 from
Ubuntu 20.
* Add some `apt-key add` to fix some apt "couldn't be verified" errors
* Generally drop SKIP_LINK=1; work-around no longer needed
* Generally `add-apt-repository` before `apt-get update` as manual testing indicated the
reverse might not work.

Travis:
* Use gcc-7 by default (remove specific gcc-7 and gcc-4.8 builds)
* TODO in later PR: fix s390x "Assembler messages: Error: invalid switch -march=z14" failure

AppVeyor:
* Completely dropped because we are dropping VS2015 support and CircleCI covers
VS >= 2017

Also local testing with old gflags (out of necessity when using ROCKSDB_NO_FBCODE=1).

Reviewed By: mrambacher

Differential Revision: D33946377

Pulled By: pdillinger

fbshipit-source-id: ae077c823905b45370a26c0103ada119459da6c1
@pdillinger pdillinger mentioned this issue Feb 7, 2022
Closed
facebook-github-bot pushed a commit that referenced this issue Feb 7, 2022
@pdillinger @facebook-github-bot
Temporary disable Travis s390x Makefile build (#9512)
c0d2d26 
Summary:
Due to some unexplained errors with gcc-7

```
Assembler messages:
Error: invalid switch -march=z14
Error: unrecognized option -march=z14
```

Relevant to #9388

Pull Request resolved: #9512

Test Plan: CI

Reviewed By: hx235

Differential Revision: D34044989

Pulled By: pdillinger

fbshipit-source-id: a5406e8f30b2b187949f75c8cee4e2a0eb976670
facebook-github-bot pushed a commit that referenced this issue Feb 17, 2022
@adamretter @facebook-github-bot
Support C++17 Docker build environments for RocksJava (#9500)
5e64407 
Summary:
See #9388 (comment)

Pull Request resolved: #9500

Reviewed By: pdillinger

Differential Revision: D34114687

Pulled By: jay-zhuang

fbshipit-source-id: 22129d99ccd0dba7e8f1b263ddc5520d939641bf
@jonathan-albrecht-ibm
Copy link
Contributor

@pdillinger, @adamretter I started looking at the s390x assembler error reported in #9512. What's happening is that the travis build machine is a z14 model (the cpu generation) running ubuntu 16.04. The g++-7 compiler that is installed from ubuntu-toolchain-r-test detects the z14 model and sets march=z14 under the hood when -march=native is used on the g++ command line but the assembler is version 2.26.1 from the binutils package which is too old to recognize march=z14.

The easiest fix for s390x would be to update the travis builds to ubuntu 18.04 which has g++ 7.5.0 as the default compiler version. I've tested it locally and it works because the assembler on 18.04 is version 2.30 which recognizes march=z14.

16.04 is out of standard support now and I don't think the assembler would ever be updated to a newer version so I would have to find a way of updating the assembler as part of the s390x build. I can work on a PR to update the build to a 18.04 environment but wanted to check if 16.04 was still required for any reason or if 20.04 would be preferred.

@jonathan-albrecht-ibm
Copy link
Contributor

Actually, I found a fix for the s390x build on 16.04. I had put a check in build_tools/build_detect_platform to check if -march=native flag is working but the way I did it, it didn't end up calling the assembler. If I fix the -march=native check, then it correctly falls back to building with -march=z196 which is recognized by the whole toolchain. The same check in CMakelists.txt is working correctly so cmake builds are passing.

I've run into problems with ppc64le and arm64 on 18.04 so I don't have that working yet. So far, testing is going well with the -march=native check fix so I'd like to create a PR for it so that s390x can be re-enabled again. I'll try to have it ready tomorrow.

Closed
@pdillinger pdillinger mentioned this issue Mar 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pdillinger pdillinger

Labels
announcement rocksdb-7.0 PRs with breaking API changes that need to land in the next major release, 7.0.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@adamretter @pdillinger @jonathan-albrecht-ibm