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

Update install instructions to use CMake #477

Merged
merged 7 commits into from
Jan 26, 2018
Merged

Conversation

nurupo
Copy link
Member

@nurupo nurupo commented Feb 16, 2017

  • Updated INSTALL.md to use CMake
  • Fixed CMakeLists.txt a bit, mainly regarding cross-compiling to Windows
  • Added Dockerfile for cross-compiling to Windows which is used in the INSTALL.md instructions

When cross-compiling toxcore to Windows and running the test suit, on i686 I get messenger test crashing and tox_many_tcp test never finishing, but on x86_64 it's just tox_many_tcp test never finishing.

TODO(@iphydf ?): make Travis cross-compile toxcore to Windows using the provided in this PR Dockerfile, so that we know the Windows cross-compilation section from INSTALL.md is still valid and also that toxcore cross-compiles for Windows successfully.

Fixes #383.


This change is Reviewable

@nurupo nurupo added the enhancement New feature for the user, not a new feature for build script label Feb 16, 2017
@iphydf iphydf modified the milestone: v0.1.7 Feb 26, 2017
@iphydf
Copy link
Member

iphydf commented Mar 2, 2017

Neat! @nurupo would you mind adding a travis build using docker that builds the image? (It's ok to say no, I'm still happy with this change).

@nurupo
Copy link
Member Author

nurupo commented Mar 3, 2017

The INSTALL.md in this PR needs to be updated, since we now support MSVC and it claims otherwise. It would also be nice to add instructions for MSVC, but there is an issue that one can't currently build toxav with MSVC as cmake doesn't know where to look for opus and vpx without relying on pkg-config. We might want to solve this before adding MSVC instructions. It might make sense to move away from using pkg-config in cmake to using the cmake's find_package() (though we could just have both). I started working on writing FindX.cmake for every Tox dependency so that find_package() could find it, but I haven't finished and probably won't come back to it in a while. I can push my progress to a branch on my fork if someone wants to take it over.

I think there also were some new cmake options added recently, which need to be added to INSTALL.md.

Something to keep in mind after this PR is merged, in that the INSTALL.md would need to be kept in syn with cmake options, their defaults, etc. It also needs to be kept in sync with the Dockerfile options and their defaults. Writing a script for auto-updating INSTALL.md might make sense.

Neat! @nurupo would you mind adding a travis build using docker that builds the image? (It's ok to say no, I'm still happy with this change).

Heh, I wouldn't really call it neat, I'm not very happy with the shell scripts, you have to read the entire thing (all the scripts) to understand what the variables are used for and where the things are done in fs, so there is that.

As far as the Travis build goes, I will try adding a Travis build for it later this week if I will have time.

@zoff99
Copy link

zoff99 commented Mar 3, 2017

Reviewed 2 of 7 files at r1.
Review status: 2 of 7 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo nurupo force-pushed the cmake-install branch 17 times, most recently from dbd4e7e to a5fb3e6 Compare March 5, 2017 10:32
@nurupo
Copy link
Member Author

nurupo commented Mar 5, 2017

@iphydf added Travis build jobs.

As mentioned earlier, INSTALL.md needs to be updated before this PR is merged.

@iphydf
Copy link
Member

iphydf commented Mar 5, 2017

Ok, let me know when it's ready.

@nurupo nurupo force-pushed the cmake-install branch 6 times, most recently from b16f3b5 to 7dc8b87 Compare January 18, 2018 02:32
@nurupo
Copy link
Member Author

nurupo commented Jan 19, 2018

@iphydf this should be ready for a final review.

Check the INSTALL instructions. Previously it said that toxcore doesn't support building with MSVC under cmake, but now it kind of does support it in a limited no-toxav sense and if you put the libraries in the exact paths cmake expects to find them at. Should I document the library paths? They are a bit of a mess, most library paths are in the current directory (can be re-rotted to any directory with CMAKE_FIND_ROOT_PATH option provided by the user), yet other are in the Program Files. It's also a bit of a mess that cmake doesn't look for toxav dependencies and thus is unable to build toxav with MSVC.

Also, is it really a good idea to have the pre-built section mentioning which distributions have toxcore in their package repository and also linking to windows nightly builds of toxcore? I don't feel especially strongly about it.

What do I do with the Travis builds? I left them unconditioned to let you see that they work. I assume you will ask me to put them under the cron condition after you are happy with the PR.

@iphydf
Copy link
Member

iphydf commented Jan 21, 2018

Reviewed 1 of 8 files at r2, 13 of 14 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.


INSTALL.md, line 3 at r4 (raw file):

# Installation instructions

These instructions will guide you through the process of building and installing toxcore library and its components, as well as getting already pre-built binaries.

"installing the toxcore library"

Also, maybe mdformat or something, make the line length not super long.


INSTALL.md, line 34 at r4 (raw file):

#### Main

This repository, although called `toxcore`, in fact contains several libraries besides `toxcore` which complement it, as well as several executables. However, note that although these are separate libraries, at the moment, when building the libraries, they are all merged into a single `toxcore` library. Here is the full list of the main components that can be built using the CMake, their dependencies and descriptions.
  1. "using the CMake" <- why the CMake?
  2. There are a lot of commas in this sentence. I don't think there should be. Let's have some native speaker look over it.
  3. One line per paragraph makes adding review comments hard.

INSTALL.md, line 50 at r4 (raw file):

| Name        | Type       | Dependencies           | Platform  | Description                                                                                                                             |
|-------------|------------|------------------------|-----------|-----------------------------------------------------------------------------------------------------------------------------------------|
| irc_syncbot | Executable | libtoxcore             | Unix-like | Bot that synchronizes IRC channel and Tox group chat (conference).                                                                      |

"synchronises an IRC channel and a Tox group chat"


INSTALL.md, line 51 at r4 (raw file):

|-------------|------------|------------------------|-----------|-----------------------------------------------------------------------------------------------------------------------------------------|
| irc_syncbot | Executable | libtoxcore             | Unix-like | Bot that synchronizes IRC channel and Tox group chat (conference).                                                                      |
| nTox        | Executable | libtoxcore, libncurses | Unix-like | Simple text-based Tox client with support of file transfers and group chat (conference). Testing program, not intended for actual use.  |

Gone.


INSTALL.md, line 55 at r4 (raw file):

| tox_sync    | Executable | libtoxcore             | Unix-like | Bittorrent-sync-like software using Tox. Syncs two directories together.                                                                |

There are also some programs that are not plugged into the CMake build system which you might want to find interesting. You would need to build those programs yourself. These programs reside in [`other/fun`](other/fun) directory.

"might want to find interesting" sounds odd. Do I want to find it interesting? Maybe just "find interesting".


INSTALL.md, line 87 at r4 (raw file):

To build the main components you need to have CMake of at least 2.8.6 version installed. You also need to have pkg-config installed, the build system uses it to find dependency libraries.

There is some experimental accommodation for building natively on Windows, i.e. without having to use MSYS/Cygwin and pkg-config, but it uses exact hardcoded paths for finding libraries and supports building only of some of txocore components, so your mileage might vary.

txocore => toxcore


INSTALL.md, line 98 at r4 (raw file):

| BOOTSTRAP_DAEMON     | Enable building of tox-bootstrapd, the DHT bootstrap node daemon. For Unix-like systems only. | ON or OFF                                  | ON                                                |
| BUILD_AV_TEST        | Build toxav test.                                                                             | ON or OFF                                  | ON                                                |
| BUILD_NTOX           | Build nTox client.                                                                            | ON or OFF                                  | OFF                                               |

Gone.


INSTALL.md, line 127 at r4 (raw file):

```sh
cmake \
  -D ENABLE_STATIC=OFF \

Prefer -DENABLE_..., without space, as we have that everywhere in the travis scripts.


INSTALL.md, line 140 at r4 (raw file):

```sh
cd build

This directory contains Makefile.am. I'm not sure we want to build from that. I usually build from _build, because that's what "make distcheck" does when using autotools, so for all projects using autotools, that is guaranteed to work.


INSTALL.md, line 168 at r4 (raw file):

```sh
apt-get update
apt-get install docker.io

Does this work? I think I need to do "apt-get install docker", not docker.io.


cmake/ModulePackage.cmake, line 99 at r4 (raw file):

  if(ENABLE_STATIC)
    install(TARGETS ${lib}_static
      RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}

No idea what this does, or what parts of the library would end up in bindir.


other/docker/windows/build_dependencies.sh, line 9 at r4 (raw file):

build()
{
    ARCH=${1}

Other scripts have 2-space indent. Should we change those or this one?


other/docker/windows/build_toxcore.sh, line 42 at r4 (raw file):

    cp /toxcore /tmp/toxcore -R
    cd /tmp/toxcore/build
    echo "

Maybe a heredoc is nicer. Your call though.


other/docker/windows/build_toxcore.sh, line 99 at r4 (raw file):

    done
    ${WINDOWS_TOOLCHAIN}-gcc -Wl,--export-all-symbols \
                                 -Wl,--out-implib=libtox.dll.a \

Less indentation here.


other/docker/windows/get_packages.sh, line 25 at r4 (raw file):

# Arch-dependent packages required for building toxcore's dependencies and toxcore itself
if [ "${SUPPORT_ARCH_i686}" = "true" ]; then
    # g++ is unnecessary, but CMake requires it when doing its check

Just fyi: g++ may become necessary soon (for tests).


Comments from Reviewable

@nurupo
Copy link
Member Author

nurupo commented Jan 22, 2018

Review status: 8 of 11 files reviewed at latest revision, 25 unresolved discussions, some commit checks failed.


INSTALL.md, line 3 at r4 (raw file):

Previously, iphydf wrote…

"installing the toxcore library"

Also, maybe mdformat or something, make the line length not super long.

What is mdformat? Can't find it in Google or Debian packages (both as package name or a file name that is contained by any package). I can put each sentence on its own line. Splitting the text to fit into 80 characters might be doable too, markdown formatting might get in the way of doing that though, since you can't split it on new line (or can you? don't think i ever tried).


INSTALL.md, line 34 at r4 (raw file):

Previously, iphydf wrote…
  1. "using the CMake" <- why the CMake?
  2. There are a lot of commas in this sentence. I don't think there should be. Let's have some native speaker look over it.
  3. One line per paragraph makes adding review comments hard.
  1. Is that incorrect? I will remove this instance of "the CMake" then. There are a few more instances of "the CMake" in the INSTALL.md that you didn't comment on which I assume are fine so I will keep those. Noun articles are hard, my language doesn't have them at all.
  2. Sure. Who is a native speaker here?
  3. Replied in the reply to your "mdformat" comment.

INSTALL.md, line 55 at r4 (raw file):

Previously, iphydf wrote…

"might want to find interesting" sounds odd. Do I want to find it interesting? Maybe just "find interesting".

Right, this doesn't sound right at all. Should be fixed now.


INSTALL.md, line 127 at r4 (raw file):

Previously, iphydf wrote…

Prefer -DENABLE_..., without space, as we have that everywhere in the travis scripts.

According to the documentation you can use both -D <var>:<type>=<value> or -D<var>:<type>=<value>. I'm preferring to use the space-separated version in the install guide purely for clarity reasons, it's easier to spot where ENABLE_STATIC comes from than -DENABLE_STATIC.


INSTALL.md, line 128 at r4 (raw file):

cmake \
  -D ENABLE_STATIC=OFF \
  -D DEBUG=ON -DCMAKE_INSTALL_PREFIX=/opt \

This line is inconsistent with the rest. Should be fixed now.


INSTALL.md, line 140 at r4 (raw file):

Previously, iphydf wrote…

This directory contains Makefile.am. I'm not sure we want to build from that. I usually build from _build, because that's what "make distcheck" does when using autotools, so for all projects using autotools, that is guaranteed to work.

I always use "build" for this, but it's a bit annoying that I can't easily rm -rf build without git getting upset about this. I will _build for the installation guide.


INSTALL.md, line 168 at r4 (raw file):
Yes, I did check it in a Ubuntu 16.04 VM in Virtual Box back when I was writing these instructions a year ago.

The docker package in Ubuntu is

System tray for KDE3/GNOME2 docklet applications

The docker.io is

Linux container runtime


cmake/ModulePackage.cmake, line 99 at r4 (raw file):

Previously, iphydf wrote…

No idea what this does, or what parts of the library would end up in bindir.

This puts runtime things into /bin. Those are executables, and in case of Windows also .dll files. We want .dll be in /bin because that's how everyone does it and because the .exe files from /bin will need those .dll in order to run. Linux and macOS, the "non-DLL platforms" as cmake documentation calls them, are not affected by this change, .so files and .dynlib files will keep going into /lib like they always did. You can read more on what RUNTIME, LIBRARY and ARCHIVE on each platform are at https://cmake.org/cmake/help/v3.10/command/install.html#installing-targets


other/docker/windows/build_dependencies.sh, line 9 at r4 (raw file):

Previously, iphydf wrote…

Other scripts have 2-space indent. Should we change those or this one?

What are you talking about? All of the 3 scripts have 4-space indentation.


other/docker/windows/get_packages.sh, line 25 at r4 (raw file):

Previously, iphydf wrote…

Just fyi: g++ may become necessary soon (for tests).

Well, based on our discussion in other PR, the C++ compiler requirement is rather artificial and can be made optional, which I think it should be. I will remove this comment to spare any confusion though.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 22, 2018

Review status: 6 of 11 files reviewed at latest revision, 18 unresolved discussions.


INSTALL.md, line 3 at r4 (raw file):

Previously, nurupo wrote…

What is mdformat? Can't find it in Google or Debian packages (both as package name or a file name that is contained by any package). I can put each sentence on its own line. Splitting the text to fit into 80 characters might be doable too, markdown formatting might get in the way of doing that though, since you can't split it on new line (or can you? don't think i ever tried).

It's ok, we can run a markdown formatter later.


INSTALL.md, line 34 at r4 (raw file):

Previously, nurupo wrote…
  1. Is that incorrect? I will remove this instance of "the CMake" then. There are a few more instances of "the CMake" in the INSTALL.md that you didn't comment on which I assume are fine so I will keep those. Noun articles are hard, my language doesn't have them at all.
  2. Sure. Who is a native speaker here?
  3. Replied in the reply to your "mdformat" comment.
  1. I think so. I didn't check very carefully, but this one stood out to me.
  2. I don't know. I think it's ok though, it's not too important. I'd rather get this submitted sooner.
  3. Another time.

Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 22, 2018

:lgtm_strong:


Reviewed 2 of 14 files at r3, 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 17 unresolved discussions.


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jan 25, 2018

@nurupo can you rebase and squash commits as appropriate? If you deem each commit to be a separate thing, then no need to squash anything.

@nurupo
Copy link
Member Author

nurupo commented Jan 25, 2018

I guess I should remove the option to compile as CXX too from the INSTALL.md.

$CMAKE_CROSSCOMPILING_EMULATOR is automatically prefixing add_test()
in CMake versions starting 3.3[1], but because we target CMake 2.8, we
can't use that and we have to add our own $CROSSCOMPILING_EMULATOR that
will prefix add_test().

[1] https://cmake.org/cmake/help/v3.3/variable/CMAKE_CROSSCOMPILING_EMULATOR.html#variable:CMAKE_CROSSCOMPILING_EMULATOR
Runtime modules, such as executables and shared libraries should be
installed into "bin" instead of "lib".
@nurupo
Copy link
Member Author

nurupo commented Jan 25, 2018

I will let Travis run this first and after it passes I will change the commit to disable Windows builds on Travis.

@nurupo
Copy link
Member Author

nurupo commented Jan 25, 2018

Alright, it passed.

@nurupo
Copy link
Member Author

nurupo commented Jan 25, 2018

Moved Travis Windows builds to cron.

@iphydf iphydf merged commit 2a5941c into TokTok:master Jan 26, 2018
@iphydf iphydf modified the milestones: v0.2.x, v0.2.0 Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature for the user, not a new feature for build script
Development

Successfully merging this pull request may close these issues.

TODO: add cmake instructions in README.md
6 participants