Fix build with MinGW #2052

orgads · 2017-03-29T14:56:00Z

There still are many warnings (most of them about invalid printf format
for long long), but it builds if FAIL_ON_WARNINGS is disabled.

yuslepukhin · 2017-03-29T18:08:04Z

@orgads Generally, looks good but you will need to fix XPRESS compression build. Don't know why this started failing but suspect because of the additional define on the compile line. If it is indeed so, I suggest you add that one for gcc-on-windows only

facebook-github-bot · 2017-03-29T19:20:29Z

@orgads updated the pull request - view changes

facebook-github-bot · 2017-03-29T19:22:20Z

@orgads updated the pull request - view changes

orgads · 2017-03-29T19:24:04Z

There. No warnings at all.

I couldn't find a proper fix for the format warnings (believe me, it's complicated ;)), so I muted them on this platform. Other platforms will report those.

orgads · 2017-03-29T19:27:58Z

CMakeLists.txt

-    COMPILE_FLAGS "-DROCKSDB_DLL -DROCKSDB_LIBRARY_EXPORTS /Fd${CMAKE_CFG_INTDIR}/${ROCKSDB_IMPORT_LIB}.pdb")
-else()
+  if(MSVC)
+    set_target_properties(${ROCKSDB_IMPORT_LIB} PROPERTIES


There is a bit of duplication here and in line 526. Can you suggest a way to reduce it? I don't use CMake often...

My PR avoids this duplication, see https://github.com/facebook/rocksdb/pull/2051/files#diff-af3b638bc2a3e6c650974192a53c7291L518

Done, thanks.

facebook-github-bot · 2017-03-29T19:28:46Z

@orgads updated the pull request - view changes

tamird · 2017-03-29T19:34:43Z

CMakeLists.txt

  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zi /nologo  /EHsc /GS /Gd /GR /GF /fp:precise /Zc:wchar_t /Zc:forScope /errorReport:queue")

  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /FC /d2Zi+ /W3 /wd4127 /wd4800 /wd4996 /wd4351")
 else()
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -W -Wextra -Wall")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wsign-compare -Wshadow -Wno-unused-parameter -Wno-unused-variable -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers")
+  if(MINGW)
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-format")


I believe this is incorrect. I ran into this warning as well, which I believe is due to a bug in mingw's inttypes.h introduced in https://sourceforge.net/p/mingw-w64/mingw-w64/ci/35d4bb429fb3bb13e9659ad4534ec784545a28ce/

The bug is that mingw-w64-headers/include/_mingw_print_push.h defines all the macros to GNU-specific values, rather than MS-specific values as the comment claims.

I tracked it down. Let me elaborate.

The problem is that _mingw_print_pop.h defines GNU values if __USE_MINGW_ANSI_STDIO is defined and is not zero. But in mingw's os_defines.h there's:

// Make sure that POSIX printf/scanf functions are activated. As // libstdc++ depends on POSIX-definitions of those functions, we define // it unconditionally. #undef __USE_MINGW_ANSI_STDIO #define __USE_MINGW_ANSI_STDIO 1

This is needed for C++ streams. And it doesn't help to set it as a gcc flag because it is being overridden.

See this discussion and this one on similar issues. Apparently the CRT works with these specifiers (%lld etc.), so these are false alarms on this case.

Anyway, even if this is/will be fixed upstream, you still need to be able to compile with older mingw versions.

Not sure what you're saying; it seems pretty clear that the author of that commit simply made a mistake in _mingw_print_pop where he sets %lld, etc, instead of the correct MS-specific macros.

Looks like you didn't follow the second link :). Let me quote:

The os_defines.h is done deliberately though, so std::cout and friends
can print 64bit integers properly.

Anyway, this discussion doesn't belong here but to the mingw mailing list. We need to live with what we have. If you find a way to use the MS defines I'll accept it.

Notice that the GNU specifiers do work as intended, even with the standard MSVCRT, so we can safely ignore the warnings.

tamird · 2017-03-29T19:36:58Z

CMakeLists.txt

-    COMPILE_FLAGS "-DROCKSDB_DLL -DROCKSDB_LIBRARY_EXPORTS /Fd${CMAKE_CFG_INTDIR}/${ROCKSDB_IMPORT_LIB}.pdb")
-else()
+  if(MSVC)
+    set_target_properties(${ROCKSDB_IMPORT_LIB} PROPERTIES


My PR avoids this duplication, see https://github.com/facebook/rocksdb/pull/2051/files#diff-af3b638bc2a3e6c650974192a53c7291L518

tamird · 2017-03-29T19:38:08Z

port/win/port_win.cc

@@ -142,7 +142,7 @@ DIR* opendir(const char* name) {
    return nullptr;
  }

-  strncpy_s(dir->entry_.d_name, dir->data_.name, strlen(dir->data_.name));
+  strcpy_s(dir->entry_.d_name, sizeof(dir->entry_.d_name), dir->data_.name);


this shouldn't have to change. I added this overload in https://sourceforge.net/p/mingw-w64/mingw-w64/ci/6ec4fb2d8c1b1abfd5aca69c382c24feb6961342.

I see, but not all users have the latest and greatest mingw, and it makes sense to support at least one version back. Moreover, using strncpy with strlen is funny.

Ah, you're correct. I think the point is that you can omit the second parameter in your call to take advantage of the template magic explained in https://msdn.microsoft.com/en-us/library/ms175759.aspx (for which I added support in the above commit).

I don't know why this was using strncpy_s before.

To clarify, I think you should be able to write

strcpy_s(dir->entry_.d_name, dir->data_.name);

here, and

strcpy_s(dirp->entry_.d_name, dirp->data_.name);

below.

Again, this will not compile with MinGW 6.2.0. Why should we support only >=6.3.0 just for a negligible style improvement?

In fact, it seems like this patch hasn't made it into any release (the latest looks to be mingw-w64 v5.0.2; 6.2.0 is the gcc version, I think).

Anyway, you're right, this is fine.

tamird · 2017-03-29T19:40:02Z

third-party/gtest-1.7.0/fused-src/gtest/gtest-all.cc

@@ -8676,8 +8676,8 @@ void Notification::WaitForNotification() {
 }

 Mutex::Mutex()
-    : type_(kDynamic),


This should go upstream, I think.

It already has: google/googletest@3330752

Heh, should probably file an issue to upgrade this to dependency to 1.8.0.

facebook-github-bot · 2017-03-29T21:13:28Z

@orgads updated the pull request - view changes

facebook-github-bot · 2017-03-29T21:19:52Z

@orgads updated the pull request - view changes

orgads · 2017-03-29T21:26:39Z

db/write_batch_test.cc

@@ -478,7 +478,7 @@ TEST_F(WriteBatchTest, DISABLED_LargeKeyValue) {
  // Insert key and value of 3GB and push total batch size to 12GB.
  static const size_t kKeyValueSize = 3221225472u;
  std::string raw(kKeyValueSize, 'A');
-  WriteBatch batch(12884901888u + 1024u);
+  WriteBatch batch(size_t(12884901888ull + 1024u));


This obviously doesn't work as expected on 32-bit platforms (regardless of my change), but this test is disabled anyway...

siying · 2017-03-30T00:42:37Z

A dumb question, does MinGW work using make instead of cmake?

tamird · 2017-03-30T00:43:15Z

Not on Windows.

EDIT: it might in the general case, but the current make-based build system doesn't support windows.

facebook-github-bot · 2017-03-30T05:33:10Z

@orgads updated the pull request - view changes

tamird · 2017-03-30T10:53:31Z

CMakeLists.txt

-endif()
-
-if(WIN32)
+  if(MSVC)


Combine this with the other MSVC block a few lines down?

facebook-github-bot · 2017-03-30T10:57:58Z

@orgads updated the pull request - view changes

siying · 2017-03-30T17:23:39Z

@yuslepukhin are you OK if I merge this PR?

tamird · 2017-03-30T17:31:58Z

@siying have you considered adding the mingw build to appveyor?

siying · 2017-03-30T17:40:58Z

@tamird you are welcome to add it, unless it takes too much time budget, or @yuslepukhin has an objection.

tamird · 2017-03-30T17:57:31Z

Sounds good. Could you share the commands you used to build and run the tests?

tamird · 2017-03-30T17:58:11Z

In the meantime, please feel free to merge this when you get the necessary approvals. I'll be happy to see it go in.

siying · 2017-03-30T17:59:43Z

@tamird the entrance is appveyor.xml https://github.com/facebook/rocksdb/blob/master/appveyor.yml . I assume you need to add the test there. @yuslepukhin knows much better than me.

tamird · 2017-03-30T18:01:41Z

Sorry, I meant the commands you ran locally to build and test on mingw.

Something like

mkdir rocksdb-build
cd rocksdb-build
cmake -G 'MSYS Makefiles' ../rocksdb
cmake --build .

?

orgads · 2017-03-30T18:55:52Z

The generator is MinGW Makefiles. The rest is right. And you need to have mingw bin directory in your PATH.

And if you want to take advantage of all your cores, execute:

cmake --build . -- -j%NUMBER_OR_PROCESSORS%

(or $NUMBER_OF_PROCESSORS, depends if you run it from bash or the command prompt)

yuslepukhin · 2017-03-30T22:47:32Z

@siying I am fine with merging it. With regards to AppVeyor, you need to check if they support it.

facebook-github-bot · 2017-03-30T23:37:50Z

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

It appears -Wformat is broken in mingw-w64's gcc at present. For example, see discussion at: facebook/rocksdb#2052 (comment) As there is no easy fix, turn off format warnings in appveyor configuration to reduce clutter in the output.

facebook-github-bot added the CLA Signed label Mar 29, 2017

orgads mentioned this pull request Mar 29, 2017

CMake: support AVX2 in MinGW #2051

Closed

orgads force-pushed the mingw branch from 1337012 to d39c396 Compare March 29, 2017 19:20

orgads force-pushed the mingw branch from d39c396 to 473700b Compare March 29, 2017 19:22

orgads commented Mar 29, 2017

View reviewed changes

orgads force-pushed the mingw branch from 473700b to 8035ba7 Compare March 29, 2017 19:28

tamird suggested changes Mar 29, 2017

View reviewed changes

orgads force-pushed the mingw branch from 8035ba7 to dd28732 Compare March 29, 2017 21:12

orgads force-pushed the mingw branch from dd28732 to 6535ba9 Compare March 29, 2017 21:19

orgads commented Mar 29, 2017

View reviewed changes

orgads force-pushed the mingw branch from 6535ba9 to 5183567 Compare March 30, 2017 05:33

tamird approved these changes Mar 30, 2017

View reviewed changes

Fix build with MinGW

7426113

orgads force-pushed the mingw branch from 5183567 to 7426113 Compare March 30, 2017 10:57

facebook-github-bot closed this in 6401a8b Mar 30, 2017

Fix build with MinGW #2052

Fix build with MinGW #2052

Conversation

orgads commented Mar 29, 2017

yuslepukhin commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

orgads commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 29, 2017

Choose a reason for hiding this comment

orgads Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 29, 2017

facebook-github-bot commented Mar 29, 2017

Choose a reason for hiding this comment

siying commented Mar 30, 2017

tamird commented Mar 30, 2017 • edited Loading

facebook-github-bot commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 30, 2017

siying commented Mar 30, 2017

tamird commented Mar 30, 2017

siying commented Mar 30, 2017

tamird commented Mar 30, 2017

tamird commented Mar 30, 2017

siying commented Mar 30, 2017

tamird commented Mar 30, 2017 • edited Loading

orgads commented Mar 30, 2017 • edited Loading

yuslepukhin commented Mar 30, 2017

facebook-github-bot commented Mar 30, 2017

orgads Mar 29, 2017 •

edited

Loading

tamird commented Mar 30, 2017 •

edited

Loading

tamird commented Mar 30, 2017 •

edited

Loading

orgads commented Mar 30, 2017 •

edited

Loading