Normalized build for MinGW-w64 #432

KOLANICH · 2018-07-17T12:14:16Z

No description provided.

tqchen · 2018-07-17T16:39:25Z

Unfortunately, this will depend on the availability of cmake. For now, we do want dmlc-core to be header only and not depending on cmake when possible

hcho3 · 2018-07-17T17:21:50Z

@tqchen Do we have commitment to support systems without CMake?

tqchen · 2018-07-17T17:24:05Z

there are always cases where we want things like single file packing(use a single source file to compile a module) even if we only use cmake. It is still a good idea to have explicit definitions on the headers(while allowing to resort to non-defaults if they are defined by flag) to support such cases

hcho3 · 2018-07-17T17:29:16Z

I still like the idea of detecting the existence of fopen64. Would it be sufficient to include a detection logic in Makefile?

tqchen · 2018-07-17T17:40:25Z

It is possible to do so in CMake, and we just need to add additional -DDMLC_USE_FOPEN64 accordingly

hcho3 · 2018-07-17T17:51:38Z

So do we ask MinGW/Cygwin users to use CMake?

KOLANICH · 2018-07-17T18:21:57Z

It is possible to do so in CMake, and we just need to add additional -DDMLC_USE_FOPEN64 accordingly

This would require detecting and setting this macrodef in all the dependencies. Creating a config is preferred because we don't need to modify build code when we wanna include these headers.

If you wanna it buildable without cmake we should recreate the templating logic via bash / sed / whatever else. But I'm not gonna do that, sorry, since I use only cmake (and if a project doesn't use cmake, I have a boilerplate to implement cmake build for it, but not all the devs accept pull requests with it, unfortunately) because it does heavy lifting itself.

tqchen · 2018-07-17T18:58:26Z

Unfortunately, do we have a requirement of amalgamation and being able to use header-only libraries, detecting things using CMake and set it on xgboost will likely resolve the XGBoost's problem, so i would recommend we go with that path

tqchen · 2018-07-17T19:09:30Z

There are very useful changes make this in this PR can be used to update CMake to run the detection in xgboost's cmake

check_symbol_exists(fopen64 stdio.h FOPEN_64_PRESENT)
if(FOPEN_64_PRESENT)
   add_definitions(-DDMLC_USE_FOPEN64=0)
endif

KOLANICH · 2018-07-17T19:12:55Z

@tqchen this breaks incapsulation :( I guess even keeping this lib header-only doesn't mean these headers shouldn't be machine-generated.

hcho3 · 2018-07-18T03:11:37Z

@KOLANICH I'm not personally familiar with the config files in CMake. How does machine generating build_config.h provide better encapsulation? All applications that include dmlc-core will now have to include dmlc/build_config.h, which to me doesn't look better than including dmlc/base.h (which happens indirectly via other dmlc headers).

KOLANICH · 2018-07-18T04:14:34Z

How does machine generating build_config.h provide better encapsulation?

The build system of the code dependent on dmlc-core doesn't have to set the macrodefs set in the build_config.h

tqchen · 2018-07-18T04:19:04Z

I think if we are to rethink things from scratch, maybe we will like to support generated header(pr modified header) when there is an install command in cmake, but when things are built in the source, we should not make the assumption that header is being generated.

The main reason is again the simple amalgamation usecase, sometimes we want to allow the user to compile a single file, which includes all the dependencies on the embedded platform and in that case we cannot make the assumption that user call cmake first

KOLANICH · 2018-07-18T07:24:41Z

The main reason is again the simple amalgamation usecase, sometimes we want to allow the user to compile a single file, which includes all the dependencies on the embedded platform and in that case we cannot make the assumption that user call cmake first

I am not sure what you mean.

Do you mean under it

we cannot make the assumption that user calls anything first

or

do you mean

we cannot make the assumption that user calls cmake, but we can assume that he calls the configure bash script or that he sets these variables manually

I guess it is the second case since amalgamation requires a bash script. Then the file should be missing, but created by a bash script.

But if you wanna it be buildable without calls of bash scripts or variables settings you can ship that file prepopulated with the default values.

tqchen · 2018-07-18T16:03:26Z

So the amalgamation use cases are pretty simple, see https://github.com/dmlc/xgboost/blob/master/amalgamation/xgboost-all0.cc

This is used to build xgboost R package, a user just has to build it with a single command line.

What I am proposing, is always default to ship the files with the default macro values(when the value is not defined), as we did before. So users don't have to do anything to do direct amalgamation compile(in certain cases add a predefined macro).

When we do want to ship things in command like make install, we use a bash script to amend the content of the default value

hcho3 · 2018-07-18T17:26:50Z

@tqchen You could use CMake to override the macro values. For instance, users of MinGW/Cygwin can now run CMake, which will override DMLC_USE_FOPEN64 as necessary.

hcho3 · 2018-07-26T17:39:28Z

@KOLANICH @tqchen I think generated header has merits. In particular, it should help fix build issues with MinGW/Cygwin. For amalgamation use case, we can ship with a pre-built build_config.h with sane default values of the macros.

tqchen · 2018-07-26T18:08:53Z

I am fine with it, as long as build_config.h is shipped in the source, and we allow cmake to override it

KOLANICH · 2018-07-31T18:42:01Z

So, are you merging it?

hcho3 · 2018-07-31T18:50:58Z

src/io/single_file_split.h

-#if defined(__FreeBSD__) && DMLC_USE_FOPEN64
-#define fopen64 std::fopen
-#endif
+#include <dmlc/build_config.h>


Can you move this line to line 10 (before all other headers)?

hcho3 · 2018-07-31T18:51:07Z

@KOLANICH Can you add a default build_config.h to the source control? Then we'll merge it.

tqchen · 2018-07-31T18:52:13Z

Please add a build_config.h that have the default options to the source. So that amalgamation will work insource without invoking cmake. This is unfortunately a case we have to support

KOLANICH · 2018-07-31T19:53:13Z

Done it, but CI build for OSX fails, because cmake is not used. Here is the code https://github.com/KOLANICH/hashcat/blame/cmake_build/.travis.yml#L16 installing CMake into travis (needs some fixes), but keep in mind that it takes several minutes. The solution may be to use a docker image, but I'm not sure if this feature available in community edition of travis.

hcho3 · 2018-07-31T19:58:00Z

include/dmlc/build_config.h

@@ -0,0 +1,6 @@
+#define FOPEN_64_PRESENT


You should remove this line, since by default FOPEN_64_PRESENT would be 0.

hcho3 · 2018-07-31T19:58:48Z

@KOLANICH No need to worry about that. See my comment in build_config.h.

KOLANICH · 2018-07-31T20:03:11Z

Done.

hcho3 · 2018-07-31T20:06:02Z

Let's exclude build_config.h from lint by modifying

dmlc-core/Makefile

Line 12 in 14409c9

    
           NOLINT_FILES = --exclude_path include/dmlc/concurrentqueue.h include/dmlc/blockingconcurrentqueue.h

to

NOLINT_FILES = --exclude_path include/dmlc/concurrentqueue.h include/dmlc/blockingconcurrentqueue.h include/dmlc/build_config.h

hcho3 · 2018-07-31T20:13:45Z

@tqchen @KOLANICH It looks like we need to decide what the default should be. I see two alternatives:

Set FOPEN_64_PRESENT=0 by default. With this option, we'd need to install CMake in Linux test, since the GNU stdio.h already contains fopen64.
Set FOPEN_64_PRESENT=1 by default. With this option, we'd need to install CMake in Mac test, since the stdio.h in Mac does not contain fopen64.

With option 1, we can simply modify .travis.yml (this line) and have apt quickly install CMake. As for option 2, I think the Mac build environment already has CMake.

KOLANICH · 2018-07-31T20:24:30Z

We just should use another name for the macrodef.

tqchen · 2018-07-31T20:29:19Z

We can still keep the old detection macros and set the default accordingly while allowing cmake to override it.

KOLANICH · 2018-07-31T20:37:10Z

have apt quickly install CMake.

Cmake in trusty repos is very old. Not sure if it is useful for us.

We can still keep the old detection macros and set the default accordingly while allowing cmake to override it.

We can.

hcho3 · 2018-08-17T18:27:02Z

@KOLANICH I'd like to do something about MinGW support (and fopen64 detection). Can I take over this pull request? You can toggle the checkbox "Allow edits from maintainers."

KOLANICH · 2018-08-20T00:38:02Z

@hcho3 done

hcho3 · 2018-09-05T06:57:15Z

@KOLANICH @tqchen I think we arrived at a happy medium: default build_config.h has some logic for people not running CMake, and you can run CMake to overwrite build_config.h.

include/dmlc/build_config.h

tqchen · 2018-09-05T17:20:21Z

I have one final comment and after changes are being made this can be merged.

Makefile

* Normalized build for MinGW-w64 * Creared default build config * Moved inclusion of build config to top in single_file_split.h * Moved inclusion of build config to top in local_filesys.cc * Commented out #define FOPEN_64_PRESENT * Excluded include/dmlc/build_config.h from lint. * Use existing logic for fopen64 when CMake is not called * Pass build_config.h to lint check * Use dmlc/base.h in local_filesys.cc to capture DMLC_USE_FOPEN64 flag

larroy · 2019-04-24T23:25:02Z

CMakeLists.txt

@@ -57,6 +61,10 @@ endif()
 # Older stdc++ enable c++11 items
 add_definitions(-D__USE_XOPEN2K8)

+check_symbol_exists(fopen64 stdio.h FOPEN_64_PRESENT)
+message(STATUS "${CMAKE_LOCAL}/build_config.h.in -> ${INCLUDE_DMLC_DIR}/build_config.h")


@larroy Some systems like MinGW have fopen() and fopen64(), and we want to choose fopen64() to use 64-bit offsets

Thanks for your response, I didn't think you would read it. The problem with this change is that build_config.h is recreated on build, which is marking the 3rdparty submodule dirty in mxnet everytime we build. Is this build_config.h changed by CMake neccessary? I would like to avoid making the repository dirty on build.

Thanks.

@larroy Originally, I had planned to have CMake create build_config.h on the fly. The header file contains special macros that are specific to platforms, e.g. existence of fopen64(). The issue was that not everyone uses CMake; many projects using dmlc-core uses GNU Make instead. So I created build_config.h containing the "default logic". Users of GNU Make will use the header as it is. On the other hand, CMake users will overwrite the header, to reflect the platform-specific details that CMake detects.

@larroy One solution would be to add build_config.h to .gitignore.

@larroy Yes, that's correct. Keep in mind that not every user of GNU Make uses Autotools ("configure")

@larroy That's weird, maybe path is off?

I think it won't work if the file is in the repo:

I tried locally without success.

https://stackoverflow.com/questions/1818895/keep-ignored-files-out-of-git-status

Got it. I'm open to suggestion as to how to better accommodate both CMake and GNU Make.

For now, you should run

git update-index --assume-unchanged include/dmlc/build_config.h

from this page

Normalized build for MinGW-w64

5994ec2

KOLANICH mentioned this pull request Jul 17, 2018

Roadmap: MinGW build support (32/64-bit) dmlc/xgboost#3436

Closed

4 tasks

KOLANICH mentioned this pull request Jul 17, 2018

Moved definitions into a config dmlc/xgboost#3479

Closed

hcho3 reviewed Jul 31, 2018

View reviewed changes

KOLANICH added 3 commits July 31, 2018 19:17

Creared default build config

34da86f

Moved inclusion of build config to top in single_file_split.h

a245822

Moved inclusion of build config to top in local_filesys.cc

8731189

hcho3 reviewed Jul 31, 2018

View reviewed changes

Commented out #define FOPEN_64_PRESENT

09781b0

Excluded include/dmlc/build_config.h from lint.

c0feb12

Use existing logic for fopen64 when CMake is not called

ac52084

tqchen requested changes Sep 5, 2018

View reviewed changes

include/dmlc/build_config.h Outdated Show resolved Hide resolved

tqchen requested changes Sep 5, 2018

View reviewed changes

Makefile Outdated Show resolved Hide resolved

hcho3 added 2 commits September 5, 2018 11:42

Pass build_config.h to lint check

58c1646

Use dmlc/base.h in local_filesys.cc to capture DMLC_USE_FOPEN64 flag

8d2f979

hcho3 merged commit 946a540 into dmlc:master Sep 5, 2018

KOLANICH deleted the fopen64_fix branch September 7, 2018 19:51

hcho3 mentioned this pull request Oct 9, 2018

Consolidate fopen64 warning #465

Merged

larroy reviewed Apr 24, 2019

View reviewed changes

hcho3 mentioned this pull request Jul 17, 2019

CMake will modify the content in 3rdparty/dmlc-core apache/mxnet#15575

Closed

hcho3 mentioned this pull request Aug 1, 2019

Running CMake should not leave unclean git trace #551

Merged

hcho3 mentioned this pull request Mar 6, 2020

Warning: Redefining fopen64 with std::fopen #576

Closed

Normalized build for MinGW-w64 #432

Normalized build for MinGW-w64 #432

Conversation

KOLANICH commented Jul 17, 2018

tqchen commented Jul 17, 2018

hcho3 commented Jul 17, 2018

tqchen commented Jul 17, 2018

hcho3 commented Jul 17, 2018

tqchen commented Jul 17, 2018

hcho3 commented Jul 17, 2018

KOLANICH commented Jul 17, 2018 • edited Loading

tqchen commented Jul 17, 2018

tqchen commented Jul 17, 2018

KOLANICH commented Jul 17, 2018

hcho3 commented Jul 18, 2018

KOLANICH commented Jul 18, 2018

tqchen commented Jul 18, 2018 • edited Loading

KOLANICH commented Jul 18, 2018 • edited Loading

tqchen commented Jul 18, 2018 • edited Loading

hcho3 commented Jul 18, 2018

hcho3 commented Jul 26, 2018

tqchen commented Jul 26, 2018

KOLANICH commented Jul 31, 2018

Choose a reason for hiding this comment

hcho3 commented Jul 31, 2018

tqchen commented Jul 31, 2018

KOLANICH commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

hcho3 commented Jul 31, 2018

KOLANICH commented Jul 31, 2018

hcho3 commented Jul 31, 2018 • edited Loading

hcho3 commented Jul 31, 2018 • edited Loading

KOLANICH commented Jul 31, 2018 • edited Loading

tqchen commented Jul 31, 2018

KOLANICH commented Jul 31, 2018

hcho3 commented Aug 17, 2018 • edited Loading

KOLANICH commented Aug 20, 2018

hcho3 commented Sep 5, 2018

tqchen commented Sep 5, 2018

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

larroy Apr 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KOLANICH commented Jul 17, 2018 •

edited

Loading

tqchen commented Jul 18, 2018 •

edited

Loading

KOLANICH commented Jul 18, 2018 •

edited

Loading

tqchen commented Jul 18, 2018 •

edited

Loading

KOLANICH commented Jul 31, 2018 •

edited

Loading

hcho3 commented Jul 31, 2018 •

edited

Loading

hcho3 commented Jul 31, 2018 •

edited

Loading

KOLANICH commented Jul 31, 2018 •

edited

Loading

hcho3 commented Aug 17, 2018 •

edited

Loading

larroy Apr 25, 2019 •

edited

Loading