7. Travis envs

[iphydf]

This change is

[iphydf]

Please comment on all files changed.

[GrayHatter]

Reviewed 2 of 17 files at r1.
Review status: 2 of 17 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

other/pkgconfig/toxav.pc.in, line 8 [r1] (raw file):

Description: Tox A/V library
Requires: opus toxcore vpx
Version: @PROJECT_VERSION@

is @PROJECT_VERSION@ going to be the ToxAV version, or the toxcore version?

---

Comments from Reviewable

[GrayHatter]

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

---

other/travis/autotools-install, line 43 [r2] (raw file):

touch lib/scanner.l

what does this line do?

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[initramfs]

Reviewed 16 of 17 files at r1.
Review status: 15 of 16 files reviewed at latest revision, 13 unresolved discussions.

---

.travis.yml, line 27 [r1] (raw file):

cache:
  directories:
    - $HOME/.cabal

Not a particularly large issue given that it works, but travis docs explicitly state that the value of $HOME should not be relied on.

---

CMakeLists.txt, line 56 [r1] (raw file):

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${VPX_CFLAGS_OTHER}")

if(WIN32)

Just a suggestion, but allow other ways for a static binary to be produced (not just compiling for win32). Say a compile flag or something.

---

other/pkgconfig/toxav.pc.in, line 8 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

is @PROJECT_VERSION@ going to be the ToxAV version, or the toxcore version?

In addition to that, perhaps explicitly make it clear by declaring it as `PROJECT_AV_VERSION` or `PROJECT_CORE_VERSION` or names of that liking.

---

other/pkgconfig/toxcore.pc.in, line 8 [r1] (raw file):

Description: Tox protocol library
Requires: libsodium
Version: @PROJECT_VERSION@

See my suggestion after GrayHatter's comment above regarding unambiguous version vars, not a big deal though.

---

other/pkgconfig/toxdns.pc.in, line 8 [r1] (raw file):

Description: Tox DNS3 library
Requires: toxcore
Version: @PROJECT_VERSION@

See my suggestion after GrayHatter's comment above regarding unambiguous version vars.

---

other/pkgconfig/toxencryptsave.pc.in, line 8 [r1] (raw file):

Description: Tox block encryption library
Requires: toxcore
Version: @PROJECT_VERSION@

See my suggestion after GrayHatter's comment above regarding unambiguous version vars.

---

other/travis/autotools-script, line 21 [r1] (raw file):

# do care whether it configures/builds at all, which is exercised by the make
# call above. Tests are executed by the cmake build.
make distcheck -j `nproc` || true

If this is to run, I suggest echoing at least a part of the comment above such that future log-dwellers don't get alarmed by this failure.

A simple

echo "The following step may fail, see: <link to this line>

Or even better, if it fails (change to a if-condition) , print a clean "fail ignored" error message

---

other/travis/hstox-after_script, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

This can be changed to set -e -u -x to guard against uninitialized vars from being used. (I've tested a travis build with -u, doesn't seem to cause issues).

---

other/travis/hstox-install, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

Same as above, change to set -e -u -x. Tested on travis.

---

other/travis/hstox-script, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

Same as above, change to set -e -u -x. Tested on travis.

---

other/travis/toxcore-after_script, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

Same as above, change to set -e -u -x. Tested on travis.

---

other/travis/toxcore-install, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

Same as above, change to set -e -u -x. Tested on travis.

---

other/travis/toxcore-script, line 3 [r1] (raw file):

#!/bin/sh

set -e -x

Same as above, change to set -e -u -x. Tested on travis.

---

Comments from Reviewable

[tux3]

Review status: 15 of 16 files reviewed at latest revision, 15 unresolved discussions.

---

CMakeLists.txt, line 60 [r1] (raw file):

else()
  set(LIBTYPE SHARED)
endif()

What's the rationale for this?
It could use a comment.

---

CMakeLists.txt, line 140 [r1] (raw file):

  if(CHECK_FOUND)
    add_executable(auto_${target} auto_tests/${target}.c)
    target_link_libraries(auto_${target}

No toxdns, will it not have tests?

---

Comments from Reviewable

[initramfs]

Review status: 15 of 16 files reviewed at latest revision, 15 unresolved discussions.

---

CMakeLists.txt, line 60 [r1] (raw file):

Previously, tux3 (Tux3) wrote…

What's the rationale for this?
It could use a comment.

Probably to save Windows tox-based programs from DLL hell, but don't quote me on it.

---

Comments from Reviewable

[iphydf]

Review status: 15 of 16 files reviewed at latest revision, 15 unresolved discussions.

---

.travis.yml, line 27 [r1] (raw file):

Previously, initramfs wrote…

Not a particularly large issue given that it works, but travis docs explicitly state that the value of $HOME should not be relied on.

Done.

---

CMakeLists.txt, line 56 [r1] (raw file):

Previously, initramfs wrote…

Just a suggestion, but allow other ways for a static binary to be produced (not just compiling for win32). Say a compile flag or something.

Done.

---

CMakeLists.txt, line 60 [r1] (raw file):

Previously, initramfs wrote…

Probably to save Windows tox-based programs from DLL hell, but don't quote me on it.

Done.

---

CMakeLists.txt, line 140 [r1] (raw file):

Previously, tux3 (Tux3) wrote…

No toxdns, will it not have tests?

I don't know whether it will, but currently it has none, so there is no linking required. I don't know the future of toxdns and I'm not terribly interested in it right now. I'm also not going to extend these tests. The toktok testing project works differently.

---

other/pkgconfig/toxav.pc.in, line 8 [r1] (raw file):

Previously, initramfs wrote…

In addition to that, perhaps explicitly make it clear by declaring it as PROJECT_AV_VERSION or PROJECT_CORE_VERSION or names of that liking.

All versions are the same in a single build. I've added a comment to the cmake file.

---

other/pkgconfig/toxcore.pc.in, line 8 [r1] (raw file):

Previously, initramfs wrote…

See my suggestion after GrayHatter's comment above regarding unambiguous version vars, not a big deal though.

Done.

---

other/pkgconfig/toxdns.pc.in, line 8 [r1] (raw file):

Previously, initramfs wrote…

See my suggestion after GrayHatter's comment above regarding unambiguous version vars.

No need to put a comment on every instance of this. My abstract thought is developed enough to be able to infer that a comment applies to all instances.

---

other/pkgconfig/toxencryptsave.pc.in, line 8 [r1] (raw file):

Previously, initramfs wrote…

See my suggestion after GrayHatter's comment above regarding unambiguous version vars.

See above.

---

other/travis/autotools-script, line 21 [r1] (raw file):

Previously, initramfs wrote…

If this is to run, I suggest echoing at least a part of the comment above such that future log-dwellers don't get alarmed by this failure.

A simple

echo "The following step may fail, see: <link to this line>

Or even better, if it fails (change to a if-condition) , print a clean "fail ignored" error message

Done.

---

other/travis/hstox-after_script, line 3 [r1] (raw file):

Previously, initramfs wrote…

This can be changed to set -e -u -x to guard against uninitialized vars from being used. (I've tested a travis build with -u, doesn't seem to cause issues).

Done.

---

other/travis/hstox-install, line 3 [r1] (raw file):

Previously, initramfs wrote…

Same as above, change to set -e -u -x. Tested on travis.

Done.

---

other/travis/hstox-script, line 3 [r1] (raw file):

Previously, initramfs wrote…

Same as above, change to set -e -u -x. Tested on travis.

Done.

---

other/travis/toxcore-after_script, line 3 [r1] (raw file):

Previously, initramfs wrote…

Same as above, change to set -e -u -x. Tested on travis.

Done.

---

other/travis/toxcore-install, line 3 [r1] (raw file):

Previously, initramfs wrote…

Same as above, change to set -e -u -x. Tested on travis.

Done.

---

other/travis/toxcore-script, line 3 [r1] (raw file):

Previously, initramfs wrote…

Same as above, change to set -e -u -x. Tested on travis.

Same as above: next time no need to copy-paste to every instance.

---

Comments from Reviewable

[zetok]

I only reviewed travis & scripts part, since I don't know about CMake or pkgconfig stuff.

Aside from what I've commented on, there are some inconsistencies with how number of build jobs is set – e.g. in toxcore-install.sh dependencies are compiled with

-j3

while in other places

-j `nproc`

is used. Personally, I think that -j$(nproc) should be used instead of the above mentioned.

AFAIK using $() instead of backticks is preferable.

---

Reviewed 1 of 1 files at r3, 1 of 10 files at r5.
Review status: 7 of 16 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

other/travis/autotools-script, line 9 [r5] (raw file):

./autogen.sh
./configure \
  --with-libsodium-libs=$CACHE_DIR/lib \

s,$CACHE_DIR/lib,$LD_LIBRARY_PATH, ?

---

other/travis/autotools-script, line 10 [r5] (raw file):

./configure \
  --with-libsodium-libs=$CACHE_DIR/lib \
  --with-libsodium-headers=$CACHE_DIR/include \

Perhaps include dir also should be sourced in env.sh?

---

other/travis/autotools-script, line 15 [r5] (raw file):

  --enable-ntox

make -j `nproc`

if env-$ENV.sh is sourced, shouldn't $MAKE be used?

Same would go for other *install.sh and *script.sh, unless I misunderstood the purpose of sourcing $MAKE?

---

other/travis/env.sh, line 7 [r5] (raw file):

export OPAMROOT=$CACHE_DIR/.opam
export LD_LIBRARY_PATH=$CACHE_DIR/lib
export PKG_CONFIG_PATH=$CACHE_DIR/lib/pkgconfig

s,$CACHE_DIR/lib/,$LD_LIBRARY_PATH/, ?

---

Comments from Reviewable

[iphydf]

$() is bash-only. I like my shell scripts bourne compatible. As for -j3, good point. I've changed it.

---

Review status: 7 of 16 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

other/travis/autotools-script, line 9 [r5] (raw file):

Previously, zetok wrote…

s,$CACHE_DIR/lib,$LD_LIBRARY_PATH, ?

$LD_LIBRARY_PATH is not a single path, but rather a `:`-separated list of lookup paths like $PATH.

---

other/travis/autotools-script, line 10 [r5] (raw file):

Previously, zetok wrote…

Perhaps include dir also should be sourced in env.sh?

You mean like $INCLUDE_DIR? I would prefer not to have too many variables. The include dir should always be at $CACHE_DIR/include. It doesn't make sense to hide that behind another variable.

---

other/travis/autotools-script, line 15 [r5] (raw file):

Previously, zetok wrote…

if env-$ENV.sh is sourced, shouldn't $MAKE be used?

Same would go for other *install.sh and *script.sh, unless I misunderstood the purpose of sourcing $MAKE?

$MAKE is for cross compilation only.

---

other/travis/env.sh, line 7 [r5] (raw file):

Previously, zetok wrote…

s,$CACHE_DIR/lib/,$LD_LIBRARY_PATH/, ?

See above.

---

Comments from Reviewable

[GrayHatter]

I think the naming scheme could be improved, but doesn't need to be.

---

Review status: 7 of 16 files reviewed at latest revision, 20 unresolved discussions.

---

CMakeLists.txt, line 140 [r1] (raw file):

Previously, iphydf wrote…

I don't know whether it will, but currently it has none, so there is no linking required. I don't know the future of toxdns and I'm not terribly interested in it right now. I'm also not going to extend these tests. The toktok testing project works differently.

No toxdns will never get tests. ToxDNS is around for legacy support. And will be dropped as soon as we can replace it with something else.

---

Comments from Reviewable

[iphydf]

I use a very similar setup on qtox. My plan is to continue doing this for a bit and then extract common logic from both, fix a naming scheme, and make it a "travis-xenv" (cross compile environment) repository that is used by both.

---

Review status: 7 of 16 files reviewed at latest revision, 19 unresolved discussions.

---

Comments from Reviewable

[zetok]

for what I've reviewed.

---

Reviewed 2 of 17 files at r1, 4 of 10 files at r5, 4 of 4 files at r6.
Review status: 15 of 16 files reviewed at latest revision, 15 unresolved discussions.

---

Comments from Reviewable

[initramfs]

Review status: 15 of 16 files reviewed at latest revision, 14 unresolved discussions.

---

other/pkgconfig/toxdns.pc.in, line 8 [r1] (raw file):

Previously, iphydf wrote…

No need to put a comment on every instance of this. My abstract thought is developed enough to be able to infer that a comment applies to all instances.

I'm just helping you flag down every instance, incase you missed one.

---

Comments from Reviewable

[tux3]

---

Reviewed 17 of 17 files at r1, 1 of 1 files at r3, 1 of 1 files at r4, 10 of 10 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

Comments from Reviewable

[GrayHatter]

CMakeLists.txt, line 60 [r1] (raw file):

Previously, iphydf wrote…

Done.

LGTM

It's because windows can't link sanely to save it's life.

---

Comments from Reviewable

[GrayHatter]

Single non-blocking comment below on CMakeLists.txt

---

Reviewed 2 of 17 files at r1, 6 of 10 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

CMakeLists.txt, line 7 [r6] (raw file):

# This version is for the entire project. All libraries (core, av, ...) move in
# versions in a synchronised way.
set(PROJECT_VERSION "0.0.0")

Non-blocking, but I'm interested in the rational for using a single "build version" instead of the version of each existing module.

---

Comments from Reviewable

[iphydf]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

CMakeLists.txt, line 7 [r6] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Non-blocking, but I'm interested in the rational for using a single "build version" instead of the version of each existing module.

Because that's the way it currently is, and this PR is not about changing versioning schemes. We can talk about that another time. Feel free to open an issue to discuss versioning.

---

Comments from Reviewable

[initramfs]

---

Reviewed 6 of 10 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

other/travis/toxcore-script, line 3 [r1] (raw file):

Previously, iphydf wrote…

Same as above: next time no need to copy-paste to every instance.

Ok. Same thing, just helping you track down instances in Reviewable so you don't need to hunt for them in the repo. I deliberately didn't flag the autotools ones because I couldn't authenticate whether they would break anything (since the build was already failing due to distcheck).

---

Comments from Reviewable