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

Two changes to make folly able to build in CygWin #568

Closed
wants to merge 3 commits into from
Closed

Two changes to make folly able to build in CygWin #568

wants to merge 3 commits into from

Conversation

cong1920
Copy link

  1. Make the generate_fingerprint_tables exec have .exe as file extension on Windows platform;
  2. Make an impl of clearenv() in C++ for Windows, CygWin and Apple platforms.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@cong1920
Copy link
Author

Actually, I was trying to add one more test case for the new impl of clearenv() on Windows/Apple platform. But I did not see the folly/experimental/test/EnvUtilTest.cpp is part of the build process. Can anyone point out how to build the EnvUtilTest.cpp and run for me? Thanks!

@cong1920
Copy link
Author

Haha, I was to file an issue for tracking this PR. Glad to see there is one, #567

@ilovezfs
Copy link

CC @Orvid

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@Orvid
Copy link
Contributor

Orvid commented Apr 3, 2017

Alright, so, Folly doesn't actually support cygwin or mingw. They may work, but are untested, as it is instead supported directly with MSVC.

I've had a diff up to do basically this since last week, but the problem is how to handle the error cases. This code currently will break if setenv has been used to set any variable to an empty string, it will also fail when any drive options are present in the environment. Right now, with the way this, and the version I have internally handles failures results in environ being in a completely unknown (but valid) state, rather than a known invalid state...

What I'm probably going to do is just disable that functionality of the environment utils for now until I can put more thought into how to fail correctly.

@Orvid
Copy link
Contributor

Orvid commented Apr 11, 2017

I ended up landing my version of clearenv in a5a6114 as we do actually need it. As we don't currently support Cygwin, I'm going to close this PR.

@Orvid Orvid closed this Apr 11, 2017
@cong1920
Copy link
Author

...reposting with my real GitHub account...

Cool, thanks! Like your implementation!
Not sure the Folly does not support Cygwin but I made it build in Cygwin though (with several hacks of course). First time to hear it supports MSVC directly. I was trying to build in MSVC from beginning but I had problems to get all Folly's dependency libs to build in MSVC so... If it builds in MSVC, can you help me get a guidance doc or something?
Are we planning to support Cygwin? If so I can make some my hacks to better code to make PRs then. Thx!

@Orvid
Copy link
Contributor

Orvid commented Apr 17, 2017

I landed the CMake files last week, so the easiest way to build on Windows is going to be via vcpkg (https://github.com/Microsoft/vcpkg), you want to install libevent:x64-windows, boost:x64-windows, openssl:x64-windows, double-conversion:x64-windows, gflags:x64-windows and glog:x64-windows. The test suite doesn't currently support being compiled without PThread support, but once it does, you'll need gtest:x64-windows to build.

Configure Folly via CMake and pass the vcpkg toolset while selecting the 64-bit platform with MSVC 2017, and build with Visual Studio.

@cong1920
Copy link
Author

Thank you, @Orvid

I did give the vcpkg a shot to install all libs you listed above. It is so easy and clean. I like it! It really saves time and energy from building all these stuff in CygWin on Windows.

Then I tried to use CMake to build the Folly along with the vcpkg toolchain set. It still needs some changes to the folly-config.h to manually turn off some macro flags to get build pass. But it is much easier than tons of hacks I had to make in CygWin.

Anyway thanks a lot! It is much better than what I have tried. I am going to compose a tutorial if anyone else wants to try building folly and the wangle on Windows. Also I will see if I can quickly learn some CMake to make those folly-config.h changes to be part of vcxproj files generating process via the CMake.

@Orvid
Copy link
Contributor

Orvid commented Apr 19, 2017

What folly-config.h changes did you need? The one the CMake build uses should be fine.

@cong1920
Copy link
Author

#define FOLLY_HAVE_BITS_FUNCTEXCEPT_H 0
#define FOLLY_HAVE_CLOCK_GETTIME 0
#define FOLLY_HAVE_CPLUS_DEMANGLE_V3_CALLBACK 0
#define FOLLY_HAVE_FEATURES_H 0
#define FOLLY_HAVE_INT128_T 0
#define FOLLY_HAVE_MEMRCHR 0
#define FOLLY_HAVE_PTHREAD_ATFORK 0
#define FOLLY_HAVE_VLA 0

Besides I have to comment out the line of "# set(CMAKE_GENERATOR_TOOLSET "v141..." in file CMakeLists.text, I read the comments above but the line itself does not do any trick but only makes CMake report an error. I still build static libs successfully with this line commented out.

Thanks!

@Orvid
Copy link
Contributor

Orvid commented Apr 19, 2017

Where did you run CMake from? None of those defines should be required for the build to work.

Which version of CMake is reporting an error?

Also, even though Folly will build without that extra CMake hack, it will fail at link time because that extra CMake hack is merging the base library and the fingerprint table support into a single folly.lib file.

@cong1920
Copy link
Author

cong1920 commented Apr 19, 2017

My CMake is from the Vcpkg's installation, which is cmake-3.8.0-rc1-win32-x86.

I am not sure the cause is from the CMake because if I do not define them 0 there would be build errors that some header files can not be found. These header files should be some non-Windows platform libs.

You could be right because there are multiple lib files in the end. I haven't tried to make Wangle build and link to the lib files, or the single folly.lib file. But if did not comment out that line this is what I got:

`d:\Code\Libs\folly\cmake-build2>d:\Toolz\CMake\bin\cmake .. -G"Visual Studio 15 2017 Win64" "-DCMAKE_TOOLCHAIN_FILE=d:\Code\Libs\vcpkg\scripts\buildsystems\vcpkg.cmake"
CMake Error at CMakeLists.txt:30 (project):
Generator

Visual Studio 15 2017 Win64

does not recognize the toolset

v141</PlatformToolset></PropertyGroup><ItemDefinitionGroup Condition="'$(ProjectName)'=='folly'"><ProjectReference><LinkLibraryDependencies>true</LinkLibraryDependencies></ProjectReference></ItemDefinitionGroup><PropertyGroup><PlatformToolset>v141

that was specified.

-- Configuring incomplete, errors occurred!
See also "D:/Code/Libs/folly/cmake-build2/CMakeFiles/CMakeOutput.log"`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants