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

CMake install #90

Open
kuzi117 opened this issue Aug 17, 2017 · 23 comments
Open

CMake install #90

kuzi117 opened this issue Aug 17, 2017 · 23 comments

Comments

@kuzi117
Copy link
Contributor

kuzi117 commented Aug 17, 2017

I've started looking at creating an install target for the api. The main problem right now is that civetweb always tries to install its executable along with it's library component. You currently disable building the executable using the EXCLUDE_FROM_ALL property, but that doesn't disable it from trying to install when we run make (and generates a warning like you mention a few lines above it). As far as I can tell, this is the only real barrier to creating a decent install target.

I see a couple of options, though, one which involves installing into the cmake binary dir and then we reinstall the header and library file from our cmake; another that was switching the add_subdirectory call to ExternalProject_Add and disabling the install step, then again installing the generated library and header ourselves; finally, which I think is the most reasonable approach that I can think of, so far, is to add a flag to disable the executable install. I have a patch prepared to add that to the blizzard fork civetweb master.

After applying that change to my local files I'm investigating a problem from protobuf:

CMake Error at contrib/protobuf/cmake/cmake_install.cmake:571 (file):
  file INSTALL cannot find
  "/home/braedy/dev/cpp/s2client-api/build/lib/cmake/protobuf".
Call Stack (most recent call first):
  cmake_install.cmake:38 (include)

Will update if I find something.

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

CivetWeb issue: civetweb/civetweb#505.

Will be opening a protobuf one because I think I found a problem in their cmake stuff as well.

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

Protobuf issue: protocolbuffers/protobuf#3518

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

I just realized that the submodule is on a version that's over a year old for protobuf. The install problem was fixed in this commit: protocolbuffers/protobuf@154e278. Can we consider updating that far at least? It continues to build fine for me even all the way up to master.

@AnthonyBrunasso
Copy link
Contributor

That actually sounds good to me. As long as it's still 2.7 it should be fine. I can update the submodule or you can provide a pull request. Which works better for you?

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

For protobuf (and civetweb if they accept my PR) I'd prefer you update the submodule so you can choose which version works for you. I'll let you know when/if they accept the civetweb PR so you don't need to track it.

The install script isn't quite done yet, when it's done I'll set up a PR for that. I wouldn't accept it though until both of the above issues have been addressed.

@AnthonyBrunasso
Copy link
Contributor

I appreciate the help!

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

No problem :)

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 18, 2017

@AnthonyBrunasso The PR was accepted: civetweb/civetweb#506

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 21, 2017

I've already written part of this in the documentation of my created install files but I thought further explanation was necessary here. Perhaps someone can even tell me that I've done something wrong and how to fix it. I'm going to try to explain what I went through as best I can. I'm not sure what anyone's experience with CMake is so this might end up kind of lengthy.

Part of installing a library is installing the exports for the library as well. These are used by importing projects to learn about targets created by an imported library. As far as I can understand I've set up the mechanisms for the API to correctly export its targets by setting the EXPORT property on all of the install(TARGETS ..). Unfortunately this is currently disabled due to some issues with the build process. Due to the sc2api target's dependence on CivetWeb's c-library target and the sc2renderer target's dependence on SDL2's SDL2-static target we encounter errors from cmake along the lines of install(EXPORT "SC2API_EXPORTS" ...) includes target "sc2api" which requires target "c-library" that is not in the export set.. Something similar is reported for sc2renderer and SDL2-static. Notably, this does not happen with sc2protocol and Protobuf's libprotobuf target. This is because (to the best of my knowledge) both SDL2 and CivetWeb do not themselves export targets while libprotobuf does.

I have tried adding these extra targets to our export set. Again, to the best of my knowledge, the only way to add a target to an export set is via the install command, however we can't install targets from a directory other than which they were created in, which is a design feature of cmake (the link is old but the concept remains). I looked for a way to just set the property but I couldn't find one, if someone knows I would gladly try it out.

That being said I went ahead and tried to create a solution that doesn't export targets and have had success linking against it in my own project after installing. I'm not sure the correct solution is to propagate the not-exporting-targets issue to downstream projects but if that's ok then I can generate a PR and we can begin testing it for others. Someone volunteered to test mac already, I believe, but I don't have an environment set up on windows. It might be an excuse for me to do it, but multiple confirmations that it works is obviously preferable.

If this is not ok, then I think the correct solution is add exports to both CivetWeb and SDL2. After mucking about in CivetWeb already to support this (disabling installing the executable) I'm fairly certain adding one there would be fairly easy. I haven't spent much time in SDL2's build system, just enough to confirm that it was not installing exports, but it might be just as easy.

If there are any further questions, let me know.

@kuzi117
Copy link
Contributor Author

kuzi117 commented Aug 25, 2017

I've tested this for ubuntu 16.04 and had someone test on mac. I'm testing windows right now but I'm unsure how to proceed with directories to install to. Is it acceptable to require admin privileges to install the API? It seems like the default install prefix is in C:/Program Files/SC2API/. Of course it could be installed in another directory and then provided to a client program via SC2API_DIR in their CMakeLists.txt.

@AnthonyBrunasso
Copy link
Contributor

AnthonyBrunasso commented Aug 28, 2017

It seems reasonable to install to our "Starcraft II" folder in Documents on windows. Mine is for example: C:\Users\abrunasso\Documents\StarCraft II

You can see ExecuteInfo.txt is in there. That's how the library finds the location of the SC2 binary to run. The game writes this file on startup.

Windows users will all have this directory if they have SC2 installed and run.

@mejan
Copy link

mejan commented Sep 3, 2017

May I ask is someone working on the install target my knowledge within Cmake is basically "non-existing" except from running a finished Cmake script. I would love to know if it is fixed or at the least worked on? Thanks for all you guys hard work.

@mejan
Copy link

mejan commented Sep 3, 2017

@kuzi117 Yes it is reasonable to ask for admin rights to the install (many other Open source things do). However are you still working on the install target and is it something I can help with (non-existing experience in Cmake)? Thanks for answer.

@kuzi117
Copy link
Contributor Author

kuzi117 commented Sep 3, 2017

Sorry about the radio silence. I've been leaving a job, moving, etc. I think all that needs to be done is switch a couple of paths and test it. I was hoping to do it tonight, honestly. I'll try to have the PR submitted tonight.

@alkurbatov
Copy link
Contributor

@kuzi117 please look at the latest changes in the API. There are a couple of renamings in 'generated' headers.

@kuzi117
Copy link
Contributor Author

kuzi117 commented Sep 3, 2017

They should be handled since I'm using their name generation to create the list of generated headers I new to install, but I'll make sure I'm up to date. Thanks for the heads up

@mejan
Copy link

mejan commented Sep 3, 2017

@kuzi117 You're awesome man!

@kuzi117
Copy link
Contributor Author

kuzi117 commented Sep 14, 2017

Just thought I'd status update this. Linux and Mac work as far as I can tell. Windows (as always) seems to be a huge pain in the ass. It installs but I thought I'd try and make an example project to ensure that it's a usable install... and it isn't. There's issues with Protobuf being found correctly that I'm still trying to work out. I can't devote a ton of time to this right but I am still working on it.

@AnthonyBrunasso
Copy link
Contributor

No worries! Honestly, I'd be fine accepting a pull request for a working mac/linux version even if the windows isn't quite ready yet.

@herodrigues
Copy link
Contributor

herodrigues commented Nov 5, 2017

@kuzi117 any progress on this? As @AnthonyBrunasso said, a working mac/linux version is better than nothing

@kuzi117
Copy link
Contributor Author

kuzi117 commented Nov 5, 2017

@herodrigues You're right. I keep getting side tracked though. I merge changes from master and then run tests, then forget about it so I fall behind again.

@herodrigues
Copy link
Contributor

@kuzi117 Yeah, I saw some commits behind. Keep up with this great work :)

@herodrigues
Copy link
Contributor

herodrigues commented Nov 5, 2017

This should work as a "manual install command"

After installing the StarCraft II API, run while inside s2client-api build directory:

$ sudo mkdir -p /usr/local/include
$ sudo cp -R include/sc2api /usr/local/include
$ sudo cp -R include/sc2utils /usr/local/include
$ sudo cp -R include/sc2lib /usr/local/include
$ sudo cp -R include/sc2renderer /usr/local/include
$ sudo cp -R build/generated/s2clientprotocol /usr/local/include

$ sudo cp -R contrib/protobuf/src/google /usr/local/include

$ sudo mkdir -p /usr/local/lib/sc2api
$ sudo cp build/bin/*.a /usr/local/lib/sc2api

Basically, these are the same steps described in @davechurchill/commandcenter.
However, I had to copy google/protobuf to /usr/local/include. Also, I decided to copy sc2api libraries and headers to /usr/local, which makes easier for me to setup a project using Eclipse.

jrepp pushed a commit to jrepp/s2client-api that referenced this issue Nov 18, 2017
Added utility for getting expansion locations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants