Open kuzi117 opened 7 years ago
CivetWeb issue: https://github.com/civetweb/civetweb/issues/505.
Will be opening a protobuf one because I think I found a problem in their cmake stuff as well.
~Protobuf issue: https://github.com/google/protobuf/issues/3518~
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: https://github.com/google/protobuf/commit/154e278a2f883deb62deb91e9499bc53539767b2. Can we consider updating that far at least? It continues to build fine for me even all the way up to master.
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?
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.
I appreciate the help!
No problem :)
@AnthonyBrunasso The PR was accepted: https://github.com/civetweb/civetweb/pull/506
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.
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.
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.
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.
@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.
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.
@kuzi117 please look at the latest changes in the API. There are a couple of renamings in 'generated' headers.
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
@kuzi117 You're awesome man!
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.
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.
@kuzi117 any progress on this? As @AnthonyBrunasso said, a working mac/linux version is better than nothing
@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.
@kuzi117 Yeah, I saw some commits behind. Keep up with this great work :)
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.
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:
Will update if I find something.