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

Normalized build for MinGW-w64 #432

Merged
merged 9 commits into from
Sep 5, 2018
Merged

Normalized build for MinGW-w64 #432

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5994ec2
Normalized build for MinGW-w64
KOLANICH Jul 17, 2018
34da86f
Creared default build config
KOLANICH Jul 31, 2018
a245822
Moved inclusion of build config to top in single_file_split.h
KOLANICH Jul 31, 2018
8731189
Moved inclusion of build config to top in local_filesys.cc
KOLANICH Jul 31, 2018
09781b0
Commented out #define FOPEN_64_PRESENT
KOLANICH Jul 31, 2018
c0feb12
Excluded include/dmlc/build_config.h from lint.
KOLANICH Jul 31, 2018
ac52084
Use existing logic for fopen64 when CMake is not called
hcho3 Sep 5, 2018
58c1646
Pass build_config.h to lint check
hcho3 Sep 5, 2018
8d2f979
Use dmlc/base.h in local_filesys.cc to capture DMLC_USE_FOPEN64 flag
hcho3 Sep 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,4 @@ build
# CLion
.idea
cmake-build-*
/include/dmlc/build_config.h
12 changes: 10 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/build/private/local_config.cmake)
include(${CMAKE_CURRENT_SOURCE_DIR}/build/private/local_config.cmake)
endif()

list(APPEND CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/Modules)
set(CMAKE_LOCAL "${PROJECT_SOURCE_DIR}/cmake")
list(APPEND CMAKE_MODULE_PATH ${CMAKE_LOCAL}/Modules)

include(CheckCXXSymbolExists)
include(cmake/Utils.cmake)
#include(cmake/dmlccore.cmake)

Expand All @@ -19,7 +21,9 @@ dmlccore_option(USE_OPENMP "Build with OpenMP" ON)
dmlccore_option(USE_CXX14_IF_AVAILABLE "Build with C++14 if the compiler supports it" OFF)

# include path
include_directories("include")
set(INCLUDE_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/include")
set(INCLUDE_DMLC_DIR "${INCLUDE_ROOT}/dmlc")
include_directories("${INCLUDE_ROOT}")

set(dmlccore_LINKER_LIBS "")
# HDFS configurations
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@larroy larroy Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, you should run

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

from this page

configure_file("${CMAKE_LOCAL}/build_config.h.in" "${INCLUDE_DMLC_DIR}/build_config.h")

# compile
if(MSVC)
add_definitions(-DDMLC_USE_CXX11)
Expand Down
6 changes: 6 additions & 0 deletions cmake/build_config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#cmakedefine FOPEN_64_PRESENT

#if !defined(FOPEN_64_PRESENT) && DMLC_USE_FOPEN64
#warning "Redefining fopen64 with std::fopen"
#define fopen64 std::fopen
#endif
10 changes: 2 additions & 8 deletions include/dmlc/base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,8 @@
# endif
#endif

#if DMLC_USE_FOPEN64 && \
(!defined(__GNUC__) || (defined __ANDROID__) || ((defined __MINGW32__) && !(defined __MINGW64__)))
#define fopen64 std::fopen
#endif

#ifdef __APPLE__
# define off64_t off_t
# if DMLC_USE_FOPEN64
# define fopen64 std::fopen
# endif
#endif

#ifdef _MSC_VER
Expand All @@ -198,6 +190,8 @@
#endif
#endif

#include <dmlc/build_config.h>


extern "C" {
#include <sys/types.h>
Expand Down
6 changes: 6 additions & 0 deletions include/dmlc/build_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#define FOPEN_64_PRESENT
Copy link
Contributor

Choose a reason for hiding this comment

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

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


#if !defined(FOPEN_64_PRESENT) && DMLC_USE_FOPEN64
#warning "Redefining fopen64 with std::fopen"
#define fopen64 std::fopen
#endif
5 changes: 1 addition & 4 deletions src/io/local_filesys.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Copyright by Contributors

#include <dmlc/build_config.h>
#include <dmlc/logging.h>
#include <errno.h>
extern "C" {
Expand All @@ -17,10 +18,6 @@ extern "C" {

#include "./local_filesys.h"

#if defined(__FreeBSD__) && DMLC_USE_FOPEN64
#define fopen64 std::fopen
#endif


namespace dmlc {
namespace io {
Expand Down
4 changes: 1 addition & 3 deletions src/io/single_file_split.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,14 @@
#ifndef DMLC_IO_SINGLE_FILE_SPLIT_H_
#define DMLC_IO_SINGLE_FILE_SPLIT_H_

#include <dmlc/build_config.h>
#include <dmlc/io.h>
#include <dmlc/logging.h>
#include <sys/stat.h>
#include <cstdio>
#include <string>
#include <algorithm>

#if defined(__FreeBSD__) && DMLC_USE_FOPEN64
#define fopen64 std::fopen
#endif

namespace dmlc {
namespace io {
Expand Down