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

Added support for Visual Studio (take 2) #2816

Closed
wants to merge 2 commits into from

Conversation

willyd
Copy link
Contributor

@willyd willyd commented Jul 24, 2015

This is a second attempt to add support for Visual Studio via the CMake build. (see PR #2538 for first attempt)

One of most time consuming part on Windows is obtaining the dependencies. To make this easier I created a super build with CMake that downloads all the dependencies and builds them and finally downloads and builds (my personal fork of) Caffe. See Caffe-Builder.

Here is a summary of the modifications:

  1. Modifications to the CMake build.
  2. Addition of a post build step that makes it possible to make static linking possible.
  3. Windows compatible re-implementations of several posix functions such as mktemp, mkdtemp, mkdir, usleep, etc.
  4. A few #ifdefs here and there to make the build go through on Windows.
  5. Disabled LMDB tests.

I think point 2. here is worth a little more explanation:

Since there is no equivalent of the whole-archive option for VS I use a post build event to parse the output of dumpbin.exe /SYMBOLS caffe.lib and generate a header file with #pragma comment(linker, "/include:") to force linking of layers in consuming projects. Since all statically registered objects and functions are defined through marcos I added a simple macro in common.hpp:

#ifndef _MSC_VER
#define DEFINE_FORCE_LINK_SYMBOL(classname, token)
#else
#define DEFINE_FORCE_LINK_SYMBOL(classname, token) \
  int g_caffe_force_link_##token##_##classname = 0
#endif

When parsing the output of dumpbin I look for any symbol containing caffe_force_link and add a pragma so it gets included in the consuming executable or shared library.

What works:

  • All targets build successfuly and install correctly
  • Python support works when python is installed. (Only tested 2.7)
  • All tests pass except Timer tests (in GPU mode) and LMDB tests.

What does not work:

I chose not to support shared libraries (yet) to limit the number of files changed by this PR.

I tested only with Visual Studio 2013 64 bit, Python 2.7 and CMake 3.2.

Since Travis CI does not yet have support for Windows I could setup a project on AppVeyor to
test PRs if this one is accepted.

@willyd
Copy link
Contributor Author

willyd commented Jul 27, 2015

@shelhamer @jeffdonahue @baeuml @kloudkl @akosiorek @Yangqing @BlGene @Nerei

Hi All,

I would like to get the attention of the lead developers of Caffe on this PR. I spent quite a lot of time to bring support for Windows with the current CMake build and would appreciate any comments.

Does it stand any chance of being accepted?

Thanks

@bhack
Copy link
Contributor

bhack commented Jul 27, 2015

As you can see BVLC members status it is unknown (probably on vacation). And nobody knows their activity plans. You need only to wait if you have enough time.

add_definitions(-DWITH_PYTHON_LAYER)
include_directories(SYSTEM ${PYTHON_INCLUDE_DIRS} ${NUMPY_INCLUDE_DIR} ${Boost_INCLUDE_DIRS})
list(APPEND Caffe_LINKER_LIBS ${PYTHON_LIBRARIES} ${Boost_LIBRARIES})
endif()
endif()
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Please be careful of whitespace noise.

@bhack
Copy link
Contributor

bhack commented Jul 27, 2015

@shelhamer Your ears are burning.

@shelhamer
Copy link
Member

@willyd while Caffe on Windows is completely unofficial it has a certain amount of popularity so this could help that part of the community. You could add your Caffe-Builder project to our wiki https://github.com/BVLC/caffe/wiki or even add a Windows page to help broadcast you build tool.

Introducing more ifdef guards is less than ideal but may be a necessary evil for cross-platform builds that aren't *nix. At least most of the changes are to the build itself and not the Caffe code.

As this does not interfere with the current CMake build it could be reasonable. Although the general complexity of the CMake scripts trouble me, that's not the problem addressed by this PR and that's fine.

Since the core does not use Windows this part would have to be community supported only. We'll discuss Windows at our next dev meeting.

@bhack
Copy link
Contributor

bhack commented Jul 27, 2015

@shelhamer Remember of #2313 at the next dev meeting.

@eelstork
Copy link
Contributor

I will try this and report findings. @willyd investigating Appveyor support based on your propositions.

configure_file("${PROJECT_SOURCE_DIR}/cmake/Templates/export.hpp.in"
${export_header} @ONLY)
# define preprocessor macro so that we will not include the generated forcelink header
target_compile_definitions(caffe PRIVATE -DBUILDING_CAFFE_LIB)
Copy link

Choose a reason for hiding this comment

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

Warning. This cmake command is available since v. 2.8.11. For now minimal required cmake is 2.8.7, which is apt-get-installed by default on Ubuntu 12.04. Seems the change to be discussed with caffe authors/community.

@willyd
Copy link
Contributor Author

willyd commented Jul 28, 2015

Thanks for your feedback everyone.

@shelhamer I agree that ifdef guards are distracting. Most probably all platform dependent code should go in dedicated files. We can probably get some inspiration by looking at how other cross-platform libraries do it.

I will wait for feedback from the dev meeting.

@eelstork I did some tests on appveyor already and it works great. It is free for OSS but the build time is limited to 30 minutes, I don't know if this will be enough to build caffe with CUDA. Currently it does not have CUDA installed, so I created a support ticket.

@willyd willyd mentioned this pull request Jul 30, 2015
@shelhamer
Copy link
Member

@longjon @jeffdonahue and I discussed and we decided that for merge

  • Changes should be the least invasive possible. Concretely, we would rather rely on a cross-platform library than ifdefs where possible -- is the functionality ifdef'd presently available in boost for instance?
  • Continuous integration is needed, since we do not have the know-how or equipment to check Windows. Could you set up appveyor as you sugested?
  • Documentation should continue to make clear that Windows support is a community effort.

Thanks.

@willyd
Copy link
Contributor Author

willyd commented Aug 4, 2015

@shelhamer

Thanks for the update.

Some changes can potentially be dealt with by a cross-platform library (the creation of temporary files and directories for example) but some other are simply different headers that need to be included. I will review the changes I made and suggest alternatives.

Sure. I can take care of setting up a build on Appveyor. I may face some problems since build time is limited to 30 minutes. I will report the results here too.

Regarding CI, does Travis run GPU tests? I beleive this is impossible with Appveyor.

Documentation could include an installation guide similar to the one for Ubuntu or OS X stating clearly that the Windows port is a community effort. Or should it only be on the project Wiki to make it less official?

@shelhamer
Copy link
Member

Regarding CI, does Travis run GPU tests? I beleive this is impossible with Appveyor.

Travis does not have GPUs so our CI only covers the CPU build. GPU CI may one day be covered through NVIDIA or AWS. No worries if the Windows CI is CPU-only.

Documentation could include an installation guide similar to the one for Ubuntu or OS X stating clearly that the Windows port is a community effort.

I think bundling it would make it more visible as long as it is clear it's from the community and how to ask for help is explained accordingly. Other core devs should comment.

@yajiedesign
Copy link
Contributor

the lmdb can work fine in windows with ntfs with small fix.
add
DWORD dwTemp;
DeviceIoControl(env->me_fd,FSCTL_SET_SPARSE,NULL,0,NULL,0,&dwTemp,NULL);
to mdb_env_open After
env->me_fd = CreateFile(dpath, oflags, FILE_SHARE_READ|FILE_SHARE_WRITE,NULL, len, mode, NULL);
this method can set file to sparse.

@lukeyeager
Copy link
Contributor

Regarding the LMDB problem, see this thread for context: jnwatson/py-lmdb#85.

there might be a performance dip
jnwatson/py-lmdb#85 (comment)

We solved this in a cross-platform way for DIGITS by increasing map_size when receiving a MapFullError - NVIDIA/DIGITS#209.

@LitingLin
Copy link

@willyd I also made a Windows build.
For io.hpp, I think this way is much more elegant.

const char temp_path_template[] =
  #ifdef __unix__
    "/tmp/caffe_test.";
  #elif defined _WIN32
    "%TEMP%/caffe_test.";
  #endif

inline void MakeTempFilename(string* temp_filename) {
    temp_filename->clear();
    *temp_filename = temp_path_template + string(tmpnam(nullptr));
    FILE* fd = fopen(temp_filename->c_str(), "w");
    CHECK(fd != NULL) << "Failed to open a temporary file at: " 
            << *temp_filename << ". Error code: " << errno;
    fclose(fd);
}

inline void MakeTempDir(string* temp_dirname) {
    temp_dirname->clear();
    *temp_dirname = temp_path_template + string(tmpnam(nullptr));
    int mkdtemp_result = mkdir(temp_dirname->c_str(), 0700);
    CHECK_EQ(mkdtemp_result, 0)   << "Failed to create a temporary directory at: " 
            << *temp_dirname << ". Error code: " << errno;
}

And calling _set_fmode(_O_BINARY) can change the default file transfer mode to Binary, then it is no need to add O_BINARY flag.
My version can build shared libraries, and with fewer ifdef preprocessors. But to honest, too many files changed.
https://github.com/LitingLin/caffe

@willyd
Copy link
Contributor Author

willyd commented Aug 18, 2015

@LitingLin Thanks for letting me know. According to @shelhamer comments, I think that the best way to handle this part would be to use boost.filesystem. unique_path and temp_directory_path would do the trick.

@bhack
Copy link
Contributor

bhack commented Aug 18, 2015

@willyd Can the dependencies be builded with http://www.cmake.org/cmake/help/v3.0/module/ExternalProject.html?

@willyd
Copy link
Contributor Author

willyd commented Aug 18, 2015

@bhack Yes and no. This is what I do in caffe-builder but I used forks for most projects since most project don't have a CMake based build (I know it is possible to build projects not based on CMake with ExternalProject but I find it so much easier to make a simple CMakeLists.txt than hacking with a mix of Visual Studio solutions and Makefiles). From the list of dependencies only:

  • OpenCV
  • glfags
  • HDF5
  • (and very recently, it didn't when I created caffe-builder) glog

have CMake based builds.

  • Protobuf has a limited CMake support for Windows: https://github.com/google/protobuf/tree/master/cmake
  • Boost is easy to build via ExternalProject even though the build uses bjam.
  • LMDB does not use CMake but is easy enough to port.
  • LevelDB does not officially support Windows (at least to the best of my knowledge). I forked Bureau14 LevelDB.
  • OpenBLAS I use the precompiled binaries since the project cannot build with MSVC.
  • Snappy does not use CMake either

@eelstork
Copy link
Contributor

@willyd @LitingLin I agree. This approach will help make the source cross platform while remaining more reable and easier to extend.

@happynear
Copy link

@yajiedesign Thanks for the fix. The database has been created successfully. However, even though it occupy only about 10MB disk space, it's size is still 1TB. I can't move or copy it to another place.
Can you further fix this problem?

@LitingLin
Copy link

@happynear On Windows, only NTFS file system support sparse file, so when you move to a FAT32 partition, it must fail. I think one way is to resize the file when closing the database handle (and resize again when opening the database?). Generally, simple modification cannot fix this problem.

@happynear
Copy link

@yajiedesign @LitingLin I have added some codes (shown below) to mdb_env_close0() to set the file size to its current file pointer. It seems that there is no need to resize again when reading the database. I have tested it on MNIST, everything works well.

    LARGE_INTEGER liCurrentPosition = { 1024*1024 };//add 1MB free space
    SetFilePointerEx(env->me_fd, liCurrentPosition,
        &liCurrentPosition, FILE_CURRENT);
    if (!SetEndOfFile(env->me_fd))
        printf("set end of file error!\n");

I don't quite know whether the 1MB more space is needed. I will do more tests.

@yajiedesign
Copy link
Contributor

@happynear
I also have this idea. I believe there is a way to find out the true size of the file, but I haven't found it yet.

@yajiedesign
Copy link
Contributor

@happynear @LitingLin
I found a way to reset the file size.add it in mdb_env_close0 befor"(void)close(env->me_fd);"
https://gist.github.com/yajiedesign/91c06f0dd9ca70ed38b5

@LitingLin
Copy link

@yajiedesign @happynear
Thanks for your works.
I'm just doubt about whether it is still working when appending records to the resized database file.

@LitingLin
Copy link

@willyd You can choose one of the third party build system to support, like Ninja, since the official ms-build doesn't support parallel build for CUDA. The build time is unacceptable currently.
If lucky, the only thing to do is add -FS to CMAKE_CXX_FLAGS.
http://www.kitware.com/blog/home/post/434

@willyd
Copy link
Contributor Author

willyd commented Aug 27, 2015

@LitingLin Thanks for pointing this out. I did not know that Ninja worked with cl.exe. I gave it a try and it is much faster than VS, the build does not go through but it lasts long enough for me to see that it is using all my cores whereas VS uses one or two. Indeed VS cannot build CUDA files in parallel. In fact, CustomBuildStep's are broken in my opinion there are several issues when building CUDA with VS some are documented here: http://public.kitware.com/pipermail/cmake-developers/2011-November/014313.html. If it is not too much effort I will definitely want to support ninja too.

@FabHan
Copy link

FabHan commented Aug 27, 2015

@willyd, after this, will I still need https://github.com/willyd/caffe-builder in order to build Caffe on Windows?

Sorry for this silly question. I'm not familiar with both Windows and CMAKE.

And this PR doesn't create a VS solution or project, does it? That means I still need to create a project in VS?

@willyd
Copy link
Contributor Author

willyd commented Aug 27, 2015

@FabHan

CMake generates the VS solutions for you. So yes, this PR creates a VS solution, but via CMake. You can find plenty of tutorials on how to use CMake on google.

Yes you will need caffe-builder to build. The current CMake build has a set of custom Find.cmake modules that do not work on Windows. caffe-builder overrides them by providing its own. These files point the find_package commands to the good locations in the caffe-builder build tree.

If you have any other questions specific to caffe-builder please ask them there.

HTH

@woozzu
Copy link

woozzu commented Sep 16, 2015

Regarding LMDB, if you don't mind performance hit I modified the patch of dw/py-lmdb#85.

https://github.com/woozzu/py-lmdb/tree/win32-sparse-patch-modified

Hopefully, I think the performance hit occurs only in write mode because when db file is closing sparse file flag is cleared.

@willyd
Copy link
Contributor Author

willyd commented Sep 12, 2016

Closing superseded by #3990.

@willyd willyd closed this Sep 12, 2016
@willyd willyd deleted the visualstudio branch November 28, 2016 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.