Skip to content
This repository has been archived by the owner on Apr 25, 2022. It is now read-only.

Build on Visual Studio #19

Closed
wants to merge 2 commits into from
Closed

Build on Visual Studio #19

wants to merge 2 commits into from

Conversation

acaly
Copy link

@acaly acaly commented Jun 3, 2015

Related to #18.

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Two remaining issues:

  • add building of run_tests executable (ideally it should share common .o objects with main executable)
  • missing info in README

As a side - have you considered using waf for integrating VS? I deliberately did not use either GNU/make or cmake due to #15 .

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Additionally, cmake's files should be added to .gitignore.

@acaly
Copy link
Author

acaly commented Jun 3, 2015

Yes I will add test and description in README, as I have commented in the issue. Maybe in a few days.

Do you mean using waf is easier than others? I'm not familiar with waf, but it seems that cmake supports VS much better.

And for cmake, add build to gitignore?

@rr-
Copy link
Member

rr- commented Jun 3, 2015

I use waf because:

  • it figures dependency hierarchy by itself
  • to me, it feels much easier to use thanks to Python (makefiles are outrageous)
  • doesn't litter ./ like cmake does

Regarding .gitignore: I meant to add CMakeFiles, Makefile, cmake_install.cmake, CMakeLists.txt and all other garbage so that git status doesn't complain about untracked files after successful build. An ideal solution though would be to get cmake to output all its stuff to build/. I'm not sure if this is possible.

As a side note, I get warnings such as:

CMake Warning (dev) at CMakeLists.txt:47 (target_link_libraries):
  Link library type specifier "debug" is followed by specifier "optimized"
  instead of a library name.  The first specifier will be ignored.
This warning is for project developers.  Use -Wno-dev to suppress it.

I don't use cmake, but shouldn't the variables be initialized with library names rather than absolute paths as you suggested in previous comments?

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Assuming I fill variables with openssl iconv etc. with any text, I am able to compile it using cmake to the point of linking (where it fails due to linking errors):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 17e4293..0cbe3ac 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -85,15 +85,27 @@ source_group("src" REGULAR_EXPRESSION "src/[^.]+[.]cc$")
 # Platform dependent
 ############################

-# Windows
-
-if (WIN32)
+# warnings
+if(MSVC)
+  if(CMAKE_CXX_FLAGS MATCHES "/W[0-4]")
+    string(REGEX REPLACE "/W[0-4]" "/W4" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
+  else()
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4")
+  endif()
   # Remove warnings and errors caused by deprecated functions on VC++
   add_definitions(-D_CRT_SECURE_NO_WARNINGS)
+elseif(CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wno-long-long -pedantic")
+endif()

+# build
+if(MSVC)
   # Add src to include directories so that compiler can find includes from src.
   include_directories(SYSTEM src)

   # Also need ws2_32.lib
   target_link_libraries(arc_unpacker ws2_32.lib)
-endif (WIN32)
\ No newline at end of file
+elseif(CMAKE_COMPILER_IS_GNUCC OR CMAKE_COMPILER_IS_GNUCXX)
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -iquote${CMAKE_SOURCE_DIR}/src")
+  target_link_libraries(arc_unpacker)
+endif()

I added a few warnings and changed WIN32 check to MSVC.

@acaly
Copy link
Author

acaly commented Jun 3, 2015

CMake will take care of path to those libraries, so I think it should be path, not lib name. The warning might be caused by the empty value of paths.

With CMake, it is usually the user that should download or build dependencies. Though not so convenient, it does not mess my Windows with all kinds of dependencies and especially the PATH environment variable. (As you know, on Windows, many libraries includes their own Python, ssh, g++, and so on, and automatically add them to PATH. Really a nightmare.)

And for CMake build files, I prefer to put them in a build folder. You can use the following commands:

mkdir build
cd build
cmake ..
make

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Whoa, that's awesome to hear. Forget the part about .gitignore then (it contains build/ already).

I have all the dependencies installed. What can I do to make cmake discover them so that users don't need to edit CMakeLists to compile the project?

@acaly
Copy link
Author

acaly commented Jun 3, 2015

I think that might be difficult. CMake supports only a few libraries, and some of them don't work well, especially when you build the library yourself with VS. That's why this time I manually added variables for libraries except for boost.

And to be honest, I have only used CMake on two or three small projects. Not very experienced. Maybe I should ask someone else for help.

@acaly
Copy link
Author

acaly commented Jun 3, 2015

People never need to modify CMakeLists. The paths can be set in CMake gui or by command line options.

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Meh, that's why I like waf, it just works (at least under Linux and cygwin).
Would it be alright with you if we moved CMakeLists.txt to extra/, or do you want to keep it in /?

@acaly
Copy link
Author

acaly commented Jun 3, 2015

That's alright to me. At least it is possible to build with CMake. The path needs to be changed though.

@rr-
Copy link
Member

rr- commented Jun 3, 2015

Alright then, so to merge this, we'll need

  • to move CMakeLists to extra/
  • to describe how things work and what user needs to do in the README
  • building of test units (it's just another executable TBH)

After this is taken care of, I'll merge your changes.

@acaly
Copy link
Author

acaly commented Jun 3, 2015

OK.

@acaly
Copy link
Author

acaly commented Jun 29, 2015

I'm back. I noticed another error C2001 when I build tests. According to VS Feedback, it is currently impossible to use utf-8 without BOM as source file in VS. Do you mind use UTF-8 with BOM for those files, or just fix them by using "\x" in string?
Files related: exe_archive.cc, mbl_archive_test.cc.

@rr-
Copy link
Member

rr- commented Jun 29, 2015

I'd rather use \x. Using BOM with UTF-8 makes little sense.

Also not sure if this relevant, but I simplified building instructions for POSIX environments (click). Thought you might be interested.

@acaly
Copy link
Author

acaly commented Jun 29, 2015

OK. Besides, the pull request started from a early commit, and now it has conflicts. I'll create a new one.

@rr-
Copy link
Member

rr- commented Dec 18, 2015

@acaly, now that VS2015 with proper C++11 support is out, I believe things should go much smoother. I opened a branch, vs, in which I'm trying to resolve all the remaining issues (there are quite a few) and add proper CMakeLists. I'm also considering ditching waf in favor of cmake.

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

Successfully merging this pull request may close these issues.

None yet

2 participants