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

travis: add Windows cross-compilation #2107

Closed
wants to merge 1 commit into from
Closed

travis: add Windows cross-compilation #2107

wants to merge 1 commit into from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 7, 2017 •
edited
Loading

  • downcase includes for case-sensitive filesystems
  • give targets the same name (librocksdb) on all platforms

With this patch it is possible to cross-compile RocksDB for Windows
from a Linux host using mingw.

cc @yuslepukhin @orgads

@facebook-github-bot
Copy link
Contributor

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

@tamird
Copy link
Contributor Author

tamird commented Apr 7, 2017

@yiwu-arbug any comments I can address?

@yiwu-arbug
Copy link
Contributor

@yuslepukhin Is this okay to merge? Thanks!

@tamird
Copy link
Contributor Author

tamird commented Apr 13, 2017

Ping.

@tamird
Copy link
Contributor Author

tamird commented Apr 18, 2017

Hello? This is absolutely necessary for cross compilation.

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@tamird tamird changed the title MinGW: downcase includes for case-sensitive filesystems travis: add Windows cross-compilation May 3, 2017
@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@tamird
Copy link
Contributor Author

tamird commented May 4, 2017

@siying this now successfully cross compiles on Travis. Could you please take a look?

@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@tamird
Copy link
Contributor Author

tamird commented May 4, 2017

Travis failed with something unrelated, I think. Otherwise, this is green (previous iteration failed appveyor, but that's fixed now).

@tamird
travis: add Windows cross-compilation
ad453a6 
- downcase includes for case-sensitive filesystems
- give targets the same name (librocksdb) on all platforms
- check localtime_r error return
- avoid shadowing use_direct_io
@facebook-github-bot
Copy link
Contributor

@tamird updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

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

@tamird tamird deleted the downcase-includes branch May 6, 2017 08:50
@adamretter
Copy link
Collaborator

adamretter commented May 14, 2017 •
edited
Loading

@tamird @siying Unfortunately I think this has broken the Windows build. I opened an issue here - #2293. Using git bisect on CMakeFiles.txt on Windows 10, I tracked the first bad commit to fdaefa0 which was from the squash and merge of this PR.

@tamird I would like to get your thoughts on this before I jump in any deeper please...

@adamretter
Copy link
Collaborator

adamretter commented May 14, 2017 •
edited
Loading

@tamird Just to follow-up, I believe the issue is specifically here: https://github.com/facebook/rocksdb/pull/2107/files#diff-af3b638bc2a3e6c650974192a53c7291L511

Previously on Windows the static RocksDB lib was named like:

set(ROCKSDB_STATIC_LIB rocksdblib${ARTIFACT_SUFFIX})

but you changed that to:

set(ROCKSDB_STATIC_LIB rocksdb${ARTIFACT_SUFFIX})

However, elsewhere, in java/CMakeLists.txt it still expects rocksdblib${ARTIFACT_SUFFIX}, i.e. https://github.com/facebook/rocksdb/blob/master/java/CMakeLists.txt#L202

I will test with changing that to match rocksdb${ARTIFACT_SUFFIX} and see if it works...

@tamird
Copy link
Contributor Author

tamird commented May 14, 2017 via email

adamretter added a commit to adamretter/rocksdb that referenced this pull request May 15, 2017
@adamretter
Fix the RocksJava Release on Windows since facebook#2107
825cab9 
Closes facebook#2293
@adamretter adamretter mentioned this pull request May 15, 2017
Closed
@adamretter
Copy link
Collaborator

adamretter commented May 15, 2017 •
edited
Loading

@tamird I made the change and it seems to work, so I have submitted a small PR for that.

I am not quite sure how mingw-w64 works as I have never used it. I would like to see rocksjavastatic build on Travis for Windows using mingw-w64 though. Would you be able to send a PR?

facebook-github-bot pushed a commit that referenced this pull request May 15, 2017
@adamretter @facebook-github-bot
Fix the RocksJava Release on Windows
254c468 
Summary:
This was previously broken accidentally by #2107

Closes #2293
Closes #2296

Differential Revision: D5061248

Pulled By: sagar0

fbshipit-source-id: 7f58fee754723a7052d2a7f9d3d0369051c3cc5c
@maysamyabandeh
Copy link
Contributor

@tamird What does mingw cover that our the appveyor test does not already do? https://github.com/facebook/rocksdb/blob/master/appveyor.yml

@tamird
Copy link
Contributor Author

tamird commented Jan 23, 2018

Cross-compilation.

@maysamyabandeh
Copy link
Contributor

Can you explain more? I do not see any related discussion at when this test was added and there is no proper documentation of why it should remain.

@tamird
Copy link
Contributor Author

tamird commented Jan 23, 2018

What would you like me to explain? The test checks that it is possible to compile rocksdb with mingw as that is the toolchain used to cross compile programs from Linux to Windows.

@maysamyabandeh
Copy link
Contributor

You mean a source might successfully compile on windows but not by mingw on linux?

@tamird
Copy link
Contributor Author

tamird commented Jan 23, 2018 via email •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuslepukhin yuslepukhin Awaiting requested review from yuslepukhin

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@tamird @facebook-github-bot @yiwu-arbug @adamretter @maysamyabandeh