Building on linux about commandcenter HOT 30 CLOSED

Apologies but I have not yet tested the bot on Linux. I will leave this issue open in hopes someone else can help you with the issue.

from commandcenter.

Hi @kuzi117 ,

for i in ~/s2client-api/include/; do sudo ln -s $i $(basename $i); done

Probably 'ln' is a problem here. Does it fails if you copy all headers directly?

For additional info, I have the following directory structure for sc2api and it works:

alkurbatov@Vika:local/include $ ls /opt/local/include/sc2utils
sc2_arg_parser.h            sc2_manage_process.h        sc2_property_reader.h       sc2_scan_directory.h        sc2_simple_serialization.h

alkurbatov@Vika:local/include $ ls /opt/local/include/sc2api
contrib/                  sc2_args.h                sc2_coordinator.h         sc2_map_info.h            sc2_typeenums.h
proto/                    sc2_client.h              sc2_data.h                sc2_proto_interface.h     sc2_types.h
sc2_action.h              sc2_common.h              sc2_game_settings.h       sc2_proto_to_pods.h       sc2_unit.h
sc2_agent.h               sc2_connection.h          sc2_gametypes.h           sc2_replay_observer.h
sc2_api.h                 sc2_control_interfaces.h  sc2_interfaces.h          sc2_score.h

alkurbatov@Vika:local/include $ ls /opt/local/include/sc2api/contrib/protobuf/src
Makefile.am  README.md    google/      solaris/

alkurbatov@Vika:local/include $ ls /opt/local/include/sc2api/proto
common.pb.cc   data.pb.cc     debug.pb.cc    error.pb.cc    query.pb.cc    raw.pb.cc      sc2api.pb.cc   score.pb.cc    spatial.pb.cc  ui.pb.cc
common.pb.h    data.pb.h      debug.pb.h     error.pb.h     query.pb.h     raw.pb.h       sc2api.pb.h    score.pb.h     spatial.pb.h   ui.pb.h

from commandcenter.

Please also apply the following pull-request to fix several linux build problems:
#11

from commandcenter.

@alkurbatov I'll try copying them directly, though I figured it if it was working for the kind it would've been ok. Will report back.

from commandcenter.

Turns out after I looked a little bit harder I realized that I misread the stack trace cmake gave me and it actually had found the sc2api directory after using ln. Even still it seems like ln is not the way to go with this because it involves adding links to directories that shouldn't have them. E.g. adding the path to the protobuf generated headers at s2client-api/build/generated has to be linked into s2client-api/include/ which doesn't seem like it should be done. The protobuf common stuff also needs to be relocated.

I noticed something kind of odd @davechurchill:

# Find Protobuf headers.
find_path(SC2Api_Protobuf_INCLUDE_DIR
    NAMES
        "google/protobuf/stubs/common.h"
    PATHS
        "${SC2Api_INCLUDE_DIR}/sc2api/contrib/protobuf/src"
)

I assumed that you were trying to follow the format of the precompiled headers provided by the api repo, but there doesn't exist a path such that the directory resolving to ${SC2Api_INCLUDE_DIR} contains sc2api/contrib/protobuf/src. Now that I look at it again, it's true for the previous find_path as well. Any insight as to how you came up with these paths? The precompiled headers don't have those paths and the files generated by cmake don't exist at those paths either.

from commandcenter.

Have you looked at my directory listing above? It contains everything regarding headers directory structure.

The current layout was selected just because it looks more or less ok nothing more. The protobuf sources were copied from sc2api (look at contrib folder carefully) just because it simplier then pick each header one by one or search for proper protobuf version. The latter could cause additional problems if blizzard team will modify their copy of protobuf.

from commandcenter.

@alkurbatov Yes, I know the structure above works, I'm just saying it seems like unnecessary work. I don't have a better suggestion right now. I asked about the paths chosen because I wanted to know if there was more reason behind it than arbitrary paths because it looks "ok".

from commandcenter.

The real problem here is lack of 'install' target in sc2api and requirment to have 'generated' headers and some 'protobuf' headers on the same level as the sc2api headers. I preserved the folders structure more or less similar to the sc2api just to avoid mixing headers in one folder. The rest is a matter of personal preference.

**/lib (not lib64) was selected because OS X doesn't have lib64 folders while for Linux it should be lib64 definitely. /opt was selected just because I like macports approach :)

Please feel free to adjust the build to Linux specifics. The pull-request will be highly appreciated.

from commandcenter.

Sorry, a bit of a misunderstanding, I didn't realize you wrote that cmake file. I'm looking into adding an install target in s2client-api, I feel like that's better than manually making folders and copying them over. I might come back to ask you some questions about what's required for mac install, if that's alright with you.

from commandcenter.

That will be the best solution and we can fix this findSC2Api.cmake too. Probably we should even include it into the upstream repo because it is not project specific.

Feel free to ask.

from commandcenter.

My intention was to have it merged into the blizzard repo, yes.

from commandcenter.

Hey @alkurbatov, could you test out the install on this fork here? https://github.com/kuzi117/s2client-api

from commandcenter.

alkurbatov@Vika:s2client-api/build ‹master›$ DESTDIR=tt make instal
...
Install the project...
-- Install configuration: ""
-- Installing: tt/usr/local/lib/cmake/SC2API/SC2APIConfig.cmake
-- Installing: tt/usr/local/lib/libcivetweb.a
-- Installing: tt/usr/local/include/civetweb.h
CMake Error at contrib/civetweb/src/cmake_install.cmake:43 (file):
file INSTALL cannot find
"/Users/alkurbatov/work/src/github.com/tmp/s2client-api/build/bin/civetweb".
Call Stack (most recent call first):
contrib/civetweb/cmake_install.cmake:32 (include)
cmake_install.cmake:44 (include)

from commandcenter.

Also this doesn't look right:

alkurbatov@Vika:s2client-api/build ‹master›$ ls tt/usr/local/lib/cmake/SC2API/SC2APIConfig.cmake
tt/usr/local/lib/cmake/SC2API/SC2APIConfig.cmake

This macros should be placed among other similar related cmake things otherwise a user must explicitly specify where to get it.

Neither it should depend on relative pathes in the build directory:

set(SC2API_INCLUDE_DIRS "${SC2API_CMAKE_DIR}/../../../include/sc2api")

Another problem here is inability to use a specific library because your 'find' macro put everything into one variable: SC2API_LIBRARIES

I believe you should rethink this part. Look at boost, this is really good example.

from commandcenter.

And another one small moment. You use the following string:
find_package(Protobuf REQUIRED)

Which version of Protobuf I need to install? As far as I know sc2api use specific submodule for it which means specific version not just random Protobuf from macports or rpm repo.

from commandcenter.

Ok, responding in order of comments:

I'm not sure what the DESTDIR assignment does. Are you trying to change the install directory? I believe you should be using -DCMAKE_INSTALL_PREFIX="/path/to/install".
Also the error that occurs there is the civetweb trying to install its binary. I just realized I forgot to give you a heads up about something that needs to be done first. The tags used in the contrib folder are... out of date (I just pushed an update to the tag for CivetWeb, but it still doesn't contain my fix that was pulled into the civetweb master upstream). I fixed the installing binary issue, so I believe if you cd to contrib/civetweb and checkout master it should be fixed. Similarly, protobuf is using a tag from over a year ago. Checkout master as well there to get a fix for cmake. There's a better solution that checkouts a specific commit but I'm waiting on blizzard to update the main repo.

The relative path is calculated at install time and should point to where the headers are installed (I can point you to the cmake code that makes this file, this was also the method suggested for setting this path by some official docs, I hope they have a good reason). If it doesn't I'd be both very surprised and very happy to find this problem now. I agree that setting everything into a single library variable is a bit heavy-handed. I mostly wanted to see that this works first, we can absolutely add variables that separate things better if this works to begin with.

You don't need to install protobuf. The correct version of protobuf is already installed as part of the installl process for s2client-api. I should probably include a version argument to match the version we've installed just before us but I haven't looked into it yet. If this is the sticking point for everything I'd be glad that it's only this.

from commandcenter.

DESTDIR is a usual way to test 'make install' command and don't pollute system folders (it is supported by the make utility itself). Cmake install prefix can be used here but I prefer destdir due to historical reasons. Actually both approaches can be used simultaneously.

Regarding protobuf lets check it after we will have working 'make install'. Probably I am wrong here.
Same for relative path.

Regarding submodules: which commit I need to checkout or it is ok to checkout from master?

from commandcenter.

I didn't know that. I'll be honest, I have no idea what sort of effect DESTDIR has on the results of the make install. If you know what you're doing that's fine, my only concern is that the SC2APIConfig.cmake won't find be found by a find_package in an importing cmake file. Can you add that directory to the search path for cmake? I'm worried this might obfuscate problems with a a real install for you, but I suppose we can deal with that after making sure you can link against it.

Go ahead and checkout master. That's what I have currently and it's working for me. If that doesn't work then I can grab specific commits to checkout.

from commandcenter.

DESTDIR is for testing purposes only and dont break anything. Actually it allows me to test and make proper installation after the tests without recompilation of the whole api (which I have to do for cmake install prefix). This is just good-old approach from the unix world :) I will do proper make install when it will be time to test compilation with the installed api.

Ok I'll try tomorrow.

from commandcenter.

I've updated protobuf and civetweb but still have the same error in civetweb during install.

The problem is in submodule itself, see this extract from .gitmodules:

[submodule "contrib/civetweb"]
        path = contrib/civetweb
        url = https://github.com/KevinCalderone/civetweb

Seems that Blizzard use its own fork of civetweb which don't have your fixes. Which commit should be applied? I'll cherry-pick it manually.

from commandcenter.

You're right, I've confused myself this time. Here's the PR: https://github.com/civetweb/civetweb/pull/506/commits

from commandcenter.

Now it works, looks good.

The only thing I don't like here is that all autogenerated headers (i.e. protobuf) were placed directly into /usr/local/include. Probably there should be some dedicated folder.

from commandcenter.

Thank you all for going so deep into this :)

from commandcenter.

Sorry, we're probably spamming you with notifications.
@alkurbatov Does it work for importing as well or are you just saying it builds and installs now?

from commandcenter.

@kuzi117 Import works too, I've checked on the clean environment. Same for linking.

The only two minor suggestion here:
a. Add a 'status' message regarding sc2api found. Right know it prints nothing during cmake phase.
b. Include path is really complex :) /usr/local/lib/cmake/SC2API/../../../include/sc2api

from commandcenter.

I'd love to add a message saying something like "Found SC2API version x.x.x", but I don't think there's a version string anywhere I can get at.
I'm trying to test out an addition to the config but I'm getting the errors in Blizzard/s2client-api#108. I've pushed a change that makes the path absolute, though I feel like it should actually be done a different way. Would you mind testing it now?

from commandcenter.

I believe it will be enough to print "Found SC2API", we don't need the version numbers because the api has no numbers right now. The only thing that could be used at this point is a hash of the last git commit.

The path looks good now:
/usr/local/include/sc2api

from commandcenter.

Generally I've seen those messages as part of the including cmake rather than the included. This is because find_package creates a variable for the includer called <package name>_FOUND.

So a user of the api would include the API as:

find_package(SC2API REQUIRED)
message(STATUS "Found SC2API: ${SC2API_FOUND}")
include_directories(..)

If you really think it's a good idea I can add it no problem. Also, what version of Mac are you using so when I make this into a PR I can notify them where it's been tested.

from commandcenter.

Generally I've seen those messages as part of the including cmake rather than the included.

Usually it is cmake 'find' macro that print such message, not the user. See find_package_handle_standard_args.

In general this is cosmetic moment, I can just enable trace in cmake or print all the variables manually, but probably somebody who don't know cmake could stuck.Because it is just an assumption you are free to decide. This message could be added anytime.

      System Version: macOS 10.12.6 (16G29)
      Kernel Version: Darwin 16.7.0

from commandcenter.

Thanks. I'll notify you if I make any significant changes.

from commandcenter.