Add GitHub Actions CI workflow for CMake build #5197

FranciscoPombal · 2020-10-03T22:58:49Z

Partly addresses #5189 (and fully addresses #5189 (comment)).

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?

.github/workflows/ci.yaml

arvidn · 2020-10-04T09:57:43Z

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 · 2020-10-04T11:51:23Z

@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 · 2020-10-04T14:45:49Z

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 · 2020-10-04T14:53:49Z

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

FranciscoPombal · 2020-10-04T16:43:36Z

@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 · 2020-10-06T17:59:23Z

@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 · 2020-10-06T18:38:45Z

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 · 2020-10-06T18:56:02Z

@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 #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:

libtorrent/CMakeLists.txt

Line 607 in 89b67d3

/bigobj # increase the number of sections for obj files

.

arvidn · 2020-10-07T08:54:33Z

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

arvidn · 2020-10-10T13:24:50Z

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 · 2020-10-19T20:41:08Z

@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: boost_python debug links to python3 release microsoft/vcpkg#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 #5197 (comment). 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 · 2020-10-22T13:00:42Z

@arvidn ping, can I get some feedback on my previous comment (#5197 (comment)), please?

arvidn · 2020-10-22T17:55:52Z

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 · 2020-10-22T17:57:47Z

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 · 2020-10-22T18:25:31Z

@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 · 2020-10-22T18:40:06Z

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 · 2020-10-26T19:55:53Z

@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 #5197 (comment). 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 · 2020-10-31T23:11:37Z

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 · 2020-11-02T12:37:02Z

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

FranciscoPombal · 2020-11-02T14:54:49Z

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 · 2020-11-02T14:58:45Z

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 · 2020-11-03T00:02:55Z

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 · 2020-11-03T00:44:04Z

@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 · 2020-11-06T22:55:40Z

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

FranciscoPombal changed the base branch from RC_2_0 to RC_1_2 October 3, 2020 22:59

FranciscoPombal mentioned this pull request Oct 3, 2020

Use GitHub Actions for Windows build #5189

Closed

arvidn reviewed Oct 4, 2020

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from 288cb86 to ea4f091 Compare October 4, 2020 11:54

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from ea4f091 to 1df6642 Compare October 6, 2020 08:35

FranciscoPombal mentioned this pull request Oct 7, 2020

fix building python bindings with CMake #5201

Merged

FranciscoPombal mentioned this pull request Oct 15, 2020

Improve python cmake #4574

Merged

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch 2 times, most recently from 73bd8bd to 26ec835 Compare October 19, 2020 20:20

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from 26ec835 to 2e51e2c Compare October 19, 2020 23:57

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from 2e51e2c to 287a13c Compare October 26, 2020 19:39

FranciscoPombal marked this pull request as ready for review October 26, 2020 19:53

FranciscoPombal mentioned this pull request Oct 29, 2020

Test merge 2 #5251

Merged

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from a7d414e to cfc5343 Compare November 1, 2020 00:05

Add GitHub Actions CI for Windows builds w/ CMake

59e0f53

FranciscoPombal force-pushed the gha_windows_cmake_RC_1_2_pr branch from cfc5343 to 59e0f53 Compare November 1, 2020 00:36

arvidn added this to the 1.2.11 milestone Nov 8, 2020

AlexCovizzi mentioned this pull request Nov 11, 2020

Add workflow to compile libtorrent AlexCovizzi/torrenttv#8

Open

arvidn modified the milestones: 1.2.11, 1.2.12 Nov 27, 2020

FranciscoPombal mentioned this pull request Dec 9, 2020

Better integration of setup.py to invoke b2 #5325

Merged

AllSeeingEyeTolledEweSew mentioned this pull request Dec 20, 2020

Test merge 2 #5758

Merged

arvidn modified the milestones: 1.2.12, 1.2.13 Jan 11, 2021

arvidn modified the milestones: 1.2.13, 1.2.14 Apr 8, 2021

arvidn modified the milestones: 1.2.14, 1.2.15 Jun 7, 2021

arvidn modified the milestones: 1.2.15, 1.2.16 Dec 27, 2021

arvidn modified the milestones: 1.2.16, 1.2.17 Apr 17, 2022

arvidn closed this Feb 27, 2023

arvidn reopened this Feb 27, 2023

arvidn modified the milestones: 1.2.17, 1.2.19 Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub Actions CI workflow for CMake build #5197

Add GitHub Actions CI workflow for CMake build #5197

FranciscoPombal commented Oct 3, 2020

arvidn commented Oct 4, 2020

FranciscoPombal commented Oct 4, 2020

arvidn commented Oct 4, 2020

arvidn commented Oct 4, 2020

FranciscoPombal commented Oct 4, 2020

FranciscoPombal commented Oct 6, 2020

arvidn commented Oct 6, 2020

FranciscoPombal commented Oct 6, 2020

arvidn commented Oct 7, 2020

arvidn commented Oct 10, 2020

FranciscoPombal commented Oct 19, 2020 •

edited

Loading

FranciscoPombal commented Oct 22, 2020

arvidn commented Oct 22, 2020

arvidn commented Oct 22, 2020

FranciscoPombal commented Oct 22, 2020

arvidn commented Oct 22, 2020

FranciscoPombal commented Oct 26, 2020

arvidn commented Oct 31, 2020

arvidn commented Nov 2, 2020

FranciscoPombal commented Nov 2, 2020

arvidn commented Nov 2, 2020

arvidn commented Nov 3, 2020

FranciscoPombal commented Nov 3, 2020 •

edited

Loading

arvidn commented Nov 6, 2020

Add GitHub Actions CI workflow for CMake build #5197

Are you sure you want to change the base?

Add GitHub Actions CI workflow for CMake build #5197

Conversation

FranciscoPombal commented Oct 3, 2020

arvidn commented Oct 4, 2020

FranciscoPombal commented Oct 4, 2020

arvidn commented Oct 4, 2020

arvidn commented Oct 4, 2020

FranciscoPombal commented Oct 4, 2020

FranciscoPombal commented Oct 6, 2020

arvidn commented Oct 6, 2020

FranciscoPombal commented Oct 6, 2020

arvidn commented Oct 7, 2020

arvidn commented Oct 10, 2020

FranciscoPombal commented Oct 19, 2020 • edited Loading

FranciscoPombal commented Oct 22, 2020

arvidn commented Oct 22, 2020

arvidn commented Oct 22, 2020

FranciscoPombal commented Oct 22, 2020

arvidn commented Oct 22, 2020

FranciscoPombal commented Oct 26, 2020

arvidn commented Oct 31, 2020

arvidn commented Nov 2, 2020

FranciscoPombal commented Nov 2, 2020

arvidn commented Nov 2, 2020

arvidn commented Nov 3, 2020

FranciscoPombal commented Nov 3, 2020 • edited Loading

arvidn commented Nov 6, 2020

FranciscoPombal commented Oct 19, 2020 •

edited

Loading

FranciscoPombal commented Nov 3, 2020 •

edited

Loading