Add GitHub Actions CI workflow for CMake build

[FranciscoPombal]

Partly addresses https://github.com/arvidn/libtorrent/issues/5189 (and fully addresses https://github.com/arvidn/libtorrent/issues/5189#issuecomment-699561653).

See it in action here: https://github.com/FranciscoPombal/libtorrent/actions/runs/287100024.

@arvidn The python bindings as well as a sister PR targeting RC_2_0 will be coming in a future commit shortly. For now I would especially appreciate comments concerning the following points:

• The current workflow builds with and without deprecated functions. Is this desirable, or should I remove one of them?
• The current workflow uploads the whole build folder as an artifact. Do you want a more slimmed down version with just a few select files (e.g compile_commands.json and target_graph*), or perhaps all of the generated tools/examples executables, or something else?

[arvidn]

Is this desirable, or should I remove one of them?

I think it's good to build both. (unless it takes too long)

The current workflow uploads the whole build folder as an artifact.

Does github provide storage for artifacts? I think the thing most people would like to have uploaded would be the python module. The other build artifacts may be a bit risky as they may not be ABI compatible with the environment of someone pulling them down.

[FranciscoPombal]

@arvidn

I think it's good to build both. (unless it takes too long)

The build jobs run in parallel, so the total workflow time is that of the slowest job.

Does github provide storage for artifacts?

Yes, default retention is 90 days, according to upload-artifact: https://github.com/actions/upload-artifact/blob/27bce4eee761b5bc643f46a8dfb41b430c8d05f6/action.yml#L25

I think the thing most people would like to have uploaded would be the python module. The other build artifacts may be a bit risky as they may not be ABI compatible with the environment of someone pulling them down.

The artifacts generated as part of this workflow are not really for general use - my main intention was to provide them only for quick inspection purposes. Though in fairness, nothing stops someone from trying to actually use the artifacts produced by these builds.

I think if you want to make some artifacts actually downloadable for some kind of production use (or even "official" prereleases), you're better off defining a workflow specifically for that purpose, possibly publishing the artifacts as actual releases.

With this in mind, besides the python module, which other files would you want published in these artifacts? Or are you OK with simply the whole build folder? Since the storage is free, I'd say the only disadvantages of publishing the whole build folder are a) download times for slower connections and b) giving some people the potentially wrong idea that the executables/libraries are meant for general use.

---

I noticed a small bug, artifacts were not being named differently based on them being built with/without deprecated functions, so there was a race condition with the uploads being overwritten. That's why https://github.com/FranciscoPombal/libtorrent/actions/runs/287100024 shows only 2 artifacts instead of 4. Will force-push to fix that shortly.

[arvidn]

In a way I would be a bit cautious to upload artifacts without a good use case. "Nightly" builds may be a good one, with debug symbols. enum_if and client_test might be good binaries to have recent builds of.

For them to be easy to use, though, it would have to link statically against libtorrent and boost.

[arvidn]

how come the github actions aren't being executed as part of this PR? is it because it's a draft?

[FranciscoPombal]

@arvidn

how come the github actions aren't being executed as part of this PR? is it because it's a draft?

They will only run after this PR is merged in this repo. It will still run for draft PRs.

[FranciscoPombal]

@arvidn Ok, I made some progress with the python bindings (only Python 3, at least for now), but I'm running into some issues. These tests are being done in an identical branch, but configured to trigger builds when it is pushed, not RC_1_2. I can reproduce these results locally also:

https://github.com/FranciscoPombal/libtorrent/runs/1216222401?check_suite_focus=true https://github.com/FranciscoPombal/libtorrent/runs/1216224891?check_suite_focus=true

As you can see, in the first run, the shared,depr_fun, py_bindings,Debug fails due to too many sections in the object file (MSVC error C1128). In the second run, I patched python-libtorrent to compile with /bigobj, which fixes the issue.

Then, in the second run, one build fails due to lack of space (we can ignore it for now). All the others that fail do so at the linking step. The libraries appear to all be successfully found, but the linking fails with unresolved symbols for boost-python. I haven't found a solution for this yet.

Please advise. In particular, should I extract the /bigobj patch into a separate PR?

[arvidn]

The libraries appear to all be successfully found, but the linking fails with unresolved symbols for boost-python. I haven't found a solution for this yet.

I seem to recall that conflicting python versions can cause this, as the python API differs between 2 and 3. Is it possible the libtorrent library is built without deprecated functions, but the python binding is built with them?

Please advise. In particular, should I extract the /bigobj patch into a separate PR?

That probably makes sense. the Jamfile adds that switch to msvc already iirc.

[FranciscoPombal]

@arvidn

I seem to recall that conflicting python versions can cause this, as the python API differs between 2 and 3. Is it possible the libtorrent library is built without deprecated functions, but the python binding is built with them?

I doubt 2 vs 3 is the issue, according to the CMake configure output, only 3 seems to be used. However, something funky might be happening due to the usage of FindPythonLibs. I think I may need to revisit https://github.com/arvidn/libtorrent/pull/4574 in order to solve this, after all, FindPythonLibs has been deprecated for a long time anyway (since CMake 3.12). Do you still care about Python 2? If not, it would simplify the work a bit as well. As for the second point, I'll double-check.

That probably makes sense. the Jamfile adds that switch to msvc already iirc.

Ok, will do - I see it is already added to the main library as well anyway: https://github.com/arvidn/libtorrent/blob/89b67d3871441955ef3a379dd340b8b940cd3551/CMakeLists.txt#L607.

[arvidn]

I don't think it's worth the effort to build and test against python2.

[arvidn]

the file ci.yml, is that name important, or can it be anything? if it can be anything, I think it would be better to describe the build that's being tested. windows_cmake.yml, or something like that

[FranciscoPombal]

@arvidn

the file ci.yml, is that name important, or can it be anything? if it can be anything, I think it would be better to describe the build that's being tested. windows_cmake.yml, or something like that

Done (renamed to windows_cmake.yml).

---

Alright, with the help of some exclusions, I have managed to get it working in the latest force-push.

Here are examples of an uncached run, and a cached run, respectively:

https://github.com/FranciscoPombal/libtorrent/actions/runs/316048316 https://github.com/FranciscoPombal/libtorrent/actions/runs/316151424

About the 4 matrix exclusions (see the comments near them), there's 1 that can be lifted...

• No python+static build

  # python bindings require building with shared libs
  - build_variant: static
    python_bindings: yes_py_bindings
  

I can fix this one with the following patch:

diff --git a/bindings/python/CMakeLists.txt b/bindings/python/CMakeLists.txt
index d5cc9e51f..a6a55d6ff 100644
--- a/bindings/python/CMakeLists.txt
+++ b/bindings/python/CMakeLists.txt
@@ -41,7 +41,13 @@ set(boost-python-module-name "python${Python3_VERSION_MAJOR}${Python3_VERSION_MI
 
 find_package(Boost REQUIRED COMPONENTS ${boost-python-module-name})
 
-Python3_add_library(python-libtorrent MODULE WITH_SOABI
+if (BUILD_SHARED_LIBS)
+   Python3_add_library(python-libtorrent MODULE WITH_SOABI
+else()
+   Python3_add_library(python-libtorrent STATIC
+endif()
+
+target_sources(python-libtorrent PRIVATE
    src/module.cpp
    src/sha1_hash.cpp
    src/converters.cpp

But does it ever make sense to build the python library statically? I legitimately don't know. If not, I won't apply this patch, and leave the exclusion be.

... and 2 would ideally be lifted, but it is not possible at this time:

• static debug python bindings failing

  Known vcpkg bug: https://github.com/microsoft/vcpkg/issues/5097

• static debug build with tests is too big

  It seems there is not enough space in the runner's build partition to hold all the files generated during the build. Nothing can be done about this, we can only hope GitHub increases the available space at some point.

---

Besides the one change/patch mentioned, the only thing remaining is to adjust the files that get uploaded as artifacts at the end of the build, as mentioned in https://github.com/arvidn/libtorrent/pull/5197#discussion_r502790242. I'll do that last, after this small matter is solved, so I'm awaiting your response.

EDIT: ~about the set-env warnings - I'll also migrate away from that as much as possible, but some core actions still depend on it, so not all the set-env warnings will disappear.~ Fixed: all usages off set-env in the script itself have been upgraded to using environment files.

[FranciscoPombal]

@arvidn ping, can I get some feedback on my previous comment (https://github.com/arvidn/libtorrent/pull/5197#issuecomment-712429665), please?

[arvidn]

But does it ever make sense to build the python library statically? I legitimately don't know. If not, I won't apply this patch, and leave the exclusion be.

I can't imagine it ever making sense to build the python module (which is what I assume you're referring to) as a static library. since the whole point is to have python itself load it as a shared library/python module.

[arvidn]

I would think it would make sense to just remove the static configuration from the matrix entirely. what configurations with a static build do you think are relevant or important?

[FranciscoPombal]

@arvidn

I can't imagine it ever making sense to build the python module (which is what I assume you're referring to) as a static library. since the whole point is to have python itself load it as a shared library/python module.

Roger that.

I would think it would make sense to just remove the static configuration from the matrix entirely. what configurations with a static build do you think are relevant or important?

I use it all the time for qBittorrent builds, for example. However, we could just have two static builds, Release and Debug (with no special features), if you're sure that there isn't any circumstance in which a shared builds with tests and/or no deprecated functions can succeed but the corresponding static builds fail. We can just leave in, there is no real additional cost that it incurs besides a little more cache usage. This is not really a problem (even for the foreseeable future), since the total current cache usage by both sets of cached dependencies (shared and static) is < 800 MiB (of a total of 5 GiB, per repository).

So what do you say?

[arvidn]

oh, right. the main library needs to be buildable as static library. I think it's pretty safe to assume that a static library works if a shared library does though. Most of the issues related to this are to missing TORRENT_EXPORT on symbols, which would always work in static libraries, but break in a shared one

[FranciscoPombal]

@arvidn

Alright, this is now ready from my side. I cleaned up the warning annotations about the deprecated Actions function by updating the actions, and slimmed the uploaded artifacts significantly according to the suggestion in https://github.com/arvidn/libtorrent/pull/5197#discussion_r502790242. Here is an example test run of the final result: https://github.com/FranciscoPombal/libtorrent/actions/runs/329760926.

By the way, here is the patch to get it working for RC_2_0. It should come in handy for one of your "test merges":

diff --git a/.github/workflows/windows_cmake.yml b/.github/workflows/windows_cmake.yml
index 594a524d0..d60270517 100644
--- a/.github/workflows/windows_cmake.yml
+++ b/.github/workflows/windows_cmake.yml
@@ -2,10 +2,10 @@ name: Windows CMake build
 
 on:
   push:
-    branches: [ RC_1_2 ]
+    branches: [ RC_2_0 ]
   pull_request:
     types: [edited, opened, reopened, synchronize]
-    branches: [ RC_1_2 ]
+    branches: [ RC_2_0 ]
 
 env:
   # openssl: 1.1.1g#1
@@ -59,6 +59,8 @@ jobs:
     steps:
     - name: checkout repository
       uses: actions/[email protected]
+      with:
+        submodules: recursive
 
     - name: setup environment - static build
       if: matrix.build_variant == 'static'
@@ -114,6 +116,8 @@ jobs:
           boost-crc:${{ env.VCPKG_TRIPLET }} `
           boost-date-time:${{ env.VCPKG_TRIPLET }} `
           boost-iterator:${{ env.VCPKG_TRIPLET }} `
+          boost-logic:${{ env.VCPKG_TRIPLET }} `
+          boost-multi-index:${{ env.VCPKG_TRIPLET }} `
           boost-multiprecision:${{ env.VCPKG_TRIPLET }} `
           boost-pool:${{ env.VCPKG_TRIPLET }} `
           boost-python:${{ env.VCPKG_TRIPLET }} `
@@ -147,7 +151,7 @@ jobs:
     - name: upload artifact as zip
       uses: actions/[email protected]
       with:
-        name: libtorrent_RC_1_2-CI-Windows_x64-${{ matrix.build_variant }}-${{ matrix.build_config }}-${{ matrix.deprecated_functions }}-${{ matri      71 x.build_tests }}-${{ matrix.python_bindings }}
+        name: libtorrent_RC_2_0-CI-Windows_x64-${{ matrix.build_variant }}-${{ matrix.build_config }}-${{ matrix.deprecated_functions }}-${{ matri      72 x.build_tests }}-${{ matrix.python_bindings }}
         path: |
           cmake-build-dir/compile_commands.json
           cmake-build-dir/target_graph.dot*

[arvidn]

I tested this locally, it it works (as in, the tests are run):

$ cmake -Dbuild_tests=YES -GNinja .
$ ninja
$ ctest

[FranciscoPombal]

This is great work! I don't consider any of my comments blockers to merge, but I think some of them should probably be addressed (either before or after the merge).

Thanks, I'll try to address as much of the issues raised as possible before the merge.

It seems like the tests are never run. For configurations with yes_tests, there still aren't any tests run by ctest. See: https://github.com/FranciscoPombal/libtorrent/runs/1310756026?check_suite_focus=true

I tested this locally, it it works (as in, the tests are run):

$ cmake -Dbuild_tests=YES -GNinja .
$ ninja
$ ctest

I'll look into it.

Also, looking at the artifacts, two of them say they are about 1.7 GB, which seems quite excessive. I downloaded them to take a look, but they don't actually seem to be that big. I think they contain the most relevant files. binaries and debug information especially.

I suppose this is a github bug.

GitHub displays the uncompressed size, but automagically compresses everything when downloading, I don't think it's a bug.

[FranciscoPombal]

@arvidn

Tests are now running, but in the shared builds they all fail due to some missing DLL: https://github.com/FranciscoPombal/libtorrent/actions/runs/338647075

[arvidn]

I suspect ctest doesn't set up the library search paths correctly, to find the dependencies to the tests. maybe the tests can't be run when building shared.

[arvidn]

if you rebase on top of RC_1_2, I think some more tests will pass. I don't know how to get test_upnp to work on appveyor, so I just disabled it by default in the Jamfile

[arvidn]

I think it's fine to disable more tests as well, for now. Or are all the tests passing now?

[FranciscoPombal]

I think it's fine to disable more tests as well, for now. Or are all the tests passing now?

Not looking great still: https://github.com/FranciscoPombal/libtorrent/actions/runs/339660523. I should be able to figure something out tonight.

[arvidn]

I see. the tests linking dynamically appear to not find the DLL (so maybe those tests could be disabled). All tests using python seem to fail because python3 doesn't seem to be installed or in the PATH. On appveyor I set the PYTHON_INTERPRETER environment variable to point to where python.exe is installed. The tests will look for this value.

[arvidn]

with python version being yet another dimension in this test matrix, I think it makes even more sense to remove py and nopy as a dimension. just always build python binding (unless there's some restriction, like static builds perhaps).

I still think test and notest also doesn't make much sense as a dimension in the test matrix.

[FranciscoPombal]

@arvidn

https://github.com/FranciscoPombal/libtorrent/actions/runs/342650928

Looks like all tests are passing now, except for test_url_seed, which seems to be flaky, as it only passed in one instance of the static builds: https://github.com/FranciscoPombal/libtorrent/runs/1344968250?check_suite_focus=true

Perhaps there is some bug? The logs are there if you need to check... (failure example: https://github.com/FranciscoPombal/libtorrent/runs/1344968233?check_suite_focus=true)

---

I added two python versions to the test - the oldest supported and the newest available, which I think is reasonable. I'll start trimming down the matrix.

[arvidn]

there are still failing tests, right? My impression is it's not ready to merge right now.