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

Add s390x builds to Travis #6168

Closed
wants to merge 6 commits into from

Conversation

adamretter
Copy link
Collaborator

Highly experimental ;-)

@sangitanalkar
Copy link

sangitanalkar commented Jan 14, 2020

Hi, any updates on the PR if the changes can be merged?

@adamretter
Copy link
Collaborator Author

@sangitanalkar No progress yet, there are a couple of issues that need to be investigated including - #6236

In addition whilst we have CI for s390x. We do not have access to an s390x system for building releases on, without that we would be unable to release binaries for that platform.

@sangitanalkar
Copy link

@adamretter we could provide the s390x system, please let us know the configuration that is needed.

@adamretter
Copy link
Collaborator Author

@sangitanalkar could you contact me directly by email to discuss.

@adamretter
Copy link
Collaborator Author

I also emailed @edelsohn about getting access to a community development system.

@edelsohn
Copy link

LinuxONE Community Cloud

@adamretter adamretter force-pushed the feature/travis-s390x branch 2 times, most recently from a7a24d1 to af86eb8 Compare March 12, 2020 16:18
.travis.yml Outdated
@@ -25,6 +26,7 @@ addons:
- liblzma-dev # xv
- libzstd-dev
- zlib1g-dev
- libssl-dev # needed for CMake to download deps
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably isn't needed now that we have pre-built binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove it

.travis.yml Show resolved Hide resolved
pdillinger added a commit to pdillinger/rocksdb that referenced this pull request Mar 13, 2020
Summary: For s390x support, some updates in newer version of Align.h are
needed. Upgrading just that file as best we can, with one addition to
Portability.h and tweaking new code in Align.h to use C++11 only (no
non-trivial constexpr functions).

Test Plan: CI, further work in PR facebook#6168
facebook-github-bot pushed a commit that referenced this pull request Mar 17, 2020
Summary:
For s390x support, some updates in newer version of Align.h are
needed. Upgrading just that file as best we can, with one addition to
Portability.h and tweaking new code in Align.h to use C++11 only (no
non-trivial constexpr functions).
Pull Request resolved: #6534

Test Plan: CI, further work in PR #6168

Differential Revision: D20445942

Pulled By: pdillinger

fbshipit-source-id: 0cef3c367463c71f3123d12cdf287c573af5e342
@jonathan-albrecht-ibm
Copy link
Contributor

Hi @adamretter, I'd like to work on adding s390x support to rocksdb. I'm part of the same group that was asking about s390x support in #8642 but I'm just looking at the latest rocksdb.

I've starting with a rebased fork of adamretter:feature/travis-s390x and added a few fixes for recent changes. I'm down to three failing tests:

  1. Full/FullBloomTest.OptimizeForMemory/0
  2. DBPropertiesTest.AggregatedTableProperties
  3. DBPropertiesTest.AggregatedTableProperties

and all failures seem related to bloom filters, eg:

...
[ RUN      ] Full/FullBloomTest.OptimizeForMemory/0
/home/jalbrecht/src/rocksdb/util/bloom_test.cc:555: Failure
Expected: (total_fp_rate / double{nfilters}) >= (0.008), actual: 0.007808 vs 0.008
...
[ RUN      ] DBPropertiesTest.AggregatedTableProperties                                                                     /home/jalbrecht/src/rocksdb/db/db_properties_test.cc:208: Failure                                                           Expected: (static_cast<double>(dbl_a - dbl_b) / (dbl_a + dbl_b)) < (bias), actual: 0.167604 vs 0.15  
[ RUN      ] DBPropertiesTest.AggregatedTablePropertiesAtLevel                                                              /home/jalbrecht/src/rocksdb/db/db_properties_test.cc:208: Failure
Expected: (static_cast<double>(dbl_a - dbl_b) / (dbl_a + dbl_b)) < (bias), actual: 0.518044 vs 0.5    
...

I'm not sure where to start with these. I posted this to the facebook RocksDB Developers group but didn't get a response so not sure I picked the best forum. Any suggestions on where or who to ask would be great.

I noticed all of theses tests set a seed for the random number generator so the same sequence is always used. Just a wild guess but I'm wondering if the threshold values.(0.008, 0.15, 0.5) are not really hard thresholds and if the errors might not indicate a real problem on s390x.

Thanks! Happy to provide any more info or testing as needed.

facebook-github-bot pushed a commit that referenced this pull request Oct 22, 2021
Summary:
This PR adds support for building on s390x including updating travis CI. It uses the previous work in #6168 and adds some more changes to get all current tests (make check and jni tests) to pass. The tests were run with snappy, lz4, bzip2 and zstd all compiled in.

There are a few pieces still needed to get the travis build working that I don't think I can do. adamretter is this something you could help with?

1. A prebuilt https://rocksdb-deps.s3-us-west-2.amazonaws.com/cmake/cmake-3.14.5-Linux-s390x.deb package
2. A https://hub.docker.com/r/evolvedbinary/rocksjava s390x image

Not sure if there is more required for travis. Happy to help in any way I can.

Pull Request resolved: #8962

Reviewed By: mrambacher

Differential Revision: D31802198

Pulled By: pdillinger

fbshipit-source-id: 683511466fa6b505f85ba5a9964a268c6151f0c2
@adamretter
Copy link
Collaborator Author

@jonathan-albrecht-ibm Sorry for the delay. Would you be able to invite me again to your https://github.com/jonathan-albrecht-ibm/cmake-3.14.5-Linux-s390x-deb repository please?

@jonathan-albrecht-ibm
Copy link
Contributor

@jonathan-albrecht-ibm Sorry for the delay. Would you be able to invite me again to your https://github.com/jonathan-albrecht-ibm/cmake-3.14.5-Linux-s390x-deb repository please?

np @adamretter I've resent the invite.

@adamretter
Copy link
Collaborator Author

@jonathan-albrecht-ibm Got it. Thanks :-)

@adamretter
Copy link
Collaborator Author

@jonathan-albrecht-ibm Can this now be closed in favour of your work in #8962 ?

I have one question about your PR (#8962)... Why the arch is set to z196 as opposed to z10, I have to admit that I am not clear about the numbering of the Z architecture. If I recall, I chose z10 as it was a reasonably old one that was also supported on Travis CI - so I figured it would give us the largest possible backwards/forwards compatibility.

@jonathan-albrecht-ibm
Copy link
Contributor

@jonathan-albrecht-ibm Can this now be closed in favour of your work in #8962 ?

Yes, #8962 includes these changes so this can be closed.

I have one question about your PR (#8962)... Why the arch is set to z196 as opposed to z10, I have to admit that I am not clear about the numbering of the Z architecture. If I recall, I chose z10 as it was a reasonably old one that was also supported on Travis CI - so I figured it would give us the largest possible backwards/forwards compatibility.

Yes, its definitely a judgment call and I didn't mean to jump the gun on this. z196 is the next generation after z10 and z196 has been around since 2010. I had a look at what the distros support:

Also, I know at least golang's s390x binary dist is built for minimum z196. So my feeling is that z196 is a good default because its not likely you're going to run into a system that supports earlier than that.

@adamretter
Copy link
Collaborator Author

Thanks @jonathan-albrecht-ibm for the explanation, let's leave it as z196 for now.

@adamretter adamretter closed this Nov 8, 2021
yoori pushed a commit to yoori/rocksdb that referenced this pull request Nov 26, 2023
Summary:
This PR adds support for building on s390x including updating travis CI. It uses the previous work in facebook/rocksdb#6168 and adds some more changes to get all current tests (make check and jni tests) to pass. The tests were run with snappy, lz4, bzip2 and zstd all compiled in.

There are a few pieces still needed to get the travis build working that I don't think I can do. adamretter is this something you could help with?

1. A prebuilt https://rocksdb-deps.s3-us-west-2.amazonaws.com/cmake/cmake-3.14.5-Linux-s390x.deb package
2. A https://hub.docker.com/r/evolvedbinary/rocksjava s390x image

Not sure if there is more required for travis. Happy to help in any way I can.

Pull Request resolved: facebook/rocksdb#8962

Reviewed By: mrambacher

Differential Revision: D31802198

Pulled By: pdillinger

fbshipit-source-id: 683511466fa6b505f85ba5a9964a268c6151f0c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants