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.

Sign up for GitHub

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

Jump to bottom

Cover Linux build by GitHub Actions #31

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Cover Linux build by GitHub Actions #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83613b5
Makefile: Fix build for self-built libsignal-protocol-c
hartwork Jan 18, 2022
786cd44
Add GitHub Action for Linux build
hartwork Jan 15, 2022
0ee6971
Make GitHub Dependabot keep our GitHub Actions up to date
hartwork Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Copyright (c) 2022 Sebastian Pipping <sebastian@pipping.org>
# Licensed under the GPL v2 or later

version: 2
updates:

- package-ecosystem: "github-actions"
hartwork marked this conversation as resolved.
Show resolved Hide resolved
commit-message:
include: "scope"
prefix: "Actions"
directory: "/"
labels:
- "enhancement"
schedule:
interval: "weekly"
65 changes: 65 additions & 0 deletions .github/workflows/linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Copyright (c) 2022 Sebastian Pipping <sebastian@pipping.org>
# Licensed under the GPL v2 or later

name: Build for Linux

on:
pull_request:
push:
schedule:
- cron: '0 2 * * 5' # Every Friday at 2am
hartwork marked this conversation as resolved.
Show resolved Hide resolved

jobs:
checks:
name: Build for Linux (${{ matrix.libsignal }} libsignal)
runs-on: ubuntu-20.04
strategy:
matrix:
libsignal: ['system', 'bundled']
steps:

- uses: actions/checkout@v2.4.0
hartwork marked this conversation as resolved.
Show resolved Hide resolved

- name: Install build dependencies (all but libsignal-protocol-c)
run: |-
set -x
sudo apt-get update
sudo apt-get install --yes --no-install-recommends -V \
gcovr \
libcmocka-dev \
libgcrypt20-dev \
libglib2.0-dev \
libsqlite3-dev

- name: Install build dependency libsignal-protocol-c
if: ${{ matrix.libsignal == 'system' }}
run: |-
sudo apt-get install --yes --no-install-recommends -V \
libsignal-protocol-c-dev

- name: Fetch Git submodule for build dependency libsignal-protocol-c
if: ${{ matrix.libsignal == 'bundled' }}
run: |-
git submodule update --init --recursive

- name: Build
run: |-
set -x
make -j $(nproc) all build/libaxc-nt.a
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not just all because it doesn't cover the "no threads" version, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's why I have two targets all build/libaxc-nt.a up there 😄


- name: Test
run: |-
make coverage # includes tests
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand the makefile right, this way the tests only run against the installed dependency, not the libsignal compiled form the submodule. correct?

what do you think about building + testing the static targets without the dependency installed in one step, and then installing it to build and test the shared lib in the next step? ("step" does not necessarily mean a github action step)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i understand the makefile right, this way the tests only run against the installed dependency, not the libsignal compiled form the submodule. correct?

I confirm:

# lddtree ./test/test_client.o | fgrep signal
    libsignal-protocol-c.so.2 => /usr/lib64/libsignal-protocol-c.so.2

(Notice the .o suffix in that line. That's where the Makefile stores the ELF executable at the moment.)

what do you think about building + testing the static targets without the dependency installed in one step, and then installing it to build and test the shared lib in the next step? ("step" does not necessarily mean a github action step)

I agree that it would be nice to have the build system be flexible enough to do things like that. There are two reasons why I'd rather not change anything about that yet in this very pull request: The mission of the pull request mostly is detecting breakage in what we already have; re-placing the low-level Makefile with high-level CMake is something that I will investigate further, which would make prior non-critical build system changes a potential waste of time. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, i don't mean changing anything about the build system. afaict the test is either run against the static or dynamic build depending on whether the system package is installed:

axc/Makefile

Line 135 in 1d4454e

ifneq ($(REQPKG),)

so my suggestion was:

  1. do not install libsignal-protocol-c package
  2. build the static targets
  3. run tests
  4. clean
  5. do install the package after
  6. build the dynamic target
  7. run tests again

i agree that seems a bit silly, and just an easy workaround until there's a proper way to do it.

Copy link
Contributor Author

@hartwork hartwork Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkdr I see, I did indeed misunderstand you then. Rather than doing all of that in one linear run, I'd split that in two halves an use the GitHub Action Matrix to with a single parameter for system-wide or self-built libsignal-protocol-c. It's not hard to do with regard to GitHub Actions:

diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml
index 292f485..e054727 100644
--- a/.github/workflows/linux.yml
+++ b/.github/workflows/linux.yml
@@ -13,11 +13,14 @@ jobs:
   checks:
     name: Build for Linux
     runs-on: ubuntu-20.04
+    strategy:
+      matrix:
+        libsignal: ['system libsignal', 'bundled libsignal']
     steps:
 
     - uses: actions/checkout@v2.4.0
 
-    - name: Install build dependencies
+    - name: Install build dependencies (all but libsignal-protocol-c)
       run: |-
         set -x
         sudo apt-get update
@@ -26,9 +29,19 @@ jobs:
             libcmocka-dev \
             libgcrypt20-dev \
             libglib2.0-dev \
-            libsignal-protocol-c-dev \
             libsqlite3-dev
 
+    - name: Install build dependency libsignal-protocol-c
+      if: ${{ matrix.libsignal == 'system libsignal' }}
+      run: |-
+        sudo apt-get install --yes --no-install-recommends -V \
+            libsignal-protocol-c-dev
+
+    - name: Fetch Git submodule for build dependency libsignal-protocol-c
+      if: ${{ matrix.libsignal == 'bundled libsignal' }}
+      run: |-
+        git submodule update --init --recursive
+
     - name: Build
       run: |-
         set -x

Problem is: The self-build half of things is broken, and so we won't get a green CI before fixing the build system further this way. The symptom (full build log) is…

# make -j 2 all build/libaxc-nt.a
[..]
cc -shared -Wl,-soname,libaxc.so.0 -o build/libaxc.so -fPIC -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include   -I./lib/libsignal-protocol-c/src -std=c11 -g -Wall -Wextra -Wpedantic -Wstrict-overflow -fno-strict-aliasing -funsigned-char -fno-builtin-memset -fstack-protector-strong -Wformat -Werror=format-security src/axc.c src/axc_crypto.c src/axc_store.c -pthread -ldl -lglib-2.0 -lsqlite3 -L/usr/lib/x86_64-linux-gnu -lgcrypt ./lib/libsignal-protocol-c/build/src/libsignal-protocol-c.a -lm -D_XOPEN_SOURCE=700 -D_BSD_SOURCE -D_POSIX_SOURCE -D_GNU_SOURCE -D_DEFAULT_SOURCE
cc: error: ./lib/libsignal-protocol-c/build/src/libsignal-protocol-c.a: No such file or directory
make: *** [Makefile:99: build/libaxc.so] Error 1
[..]

… and the cause is that $(AX_PATH) is missing at some places but also needs to be fully conditional, to not interfere with the build against the system-wide library (e.g. when someone does have the Git submodule initialized)… I would personally not want us to fix that in here, because it's not a hard dependency (plus potential upcoming build system re-write).

Copy link
Contributor Author

@hartwork hartwork Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkdr I guess someone had to fix the build anyway, so I have added the matrix and a dedicated commit to fix the build for the case of self-built libsignal-protocol-c now.


- name: Install
run: |-
set -x -o pipefail
make DESTDIR="${PWD}"/ROOT install
find ROOT/ -not -type d | sort | xargs ls -l

- name: Store coverage HTML report
uses: actions/upload-artifact@v2.3.1
with:
name: coverage_${{ matrix.libsignal }}
path: coverage/
if-no-files-found: error
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
### Fixed
- Compiler warnings ([#21](https://github.com/gkdr/axc/issues/21), [#29](https://github.com/gkdr/axc/pull/29)) (thanks, [@hartwork](https://github.com/hartwork)!)
- `gcc` can now be set from env like the rest of the tools. ([#30](https://github.com/gkdr/axc/pull/30))) (thanks, [@henry-nicolas](https://github.com/henry-nicolas) and Helmut Grohne!)
- Fix the build for users without libsignal-protocol-c installed system-wide ([#31](https://github.com/gkdr/axc/pull/31)) (thanks, [@hartwork](https://github.com/hartwork)!)
### Infrastructure
- Cover Linux build by GitHub Actions CI ([#31](https://github.com/gkdr/axc/pull/31)) (thanks, [@hartwork](https://github.com/hartwork)!)

## [0.3.6] - 2021-09-06
### Fixed
Expand Down
43 changes: 21 additions & 22 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ VER_MAJ = 0
VERSION = 0.3.6

AX_DIR=./lib/libsignal-protocol-c
AX_BDIR=$(AX_DIR)/build/src
AX_PATH=$(AX_BDIR)/libsignal-protocol-c.a
AX_BDIR=$(AX_DIR)/build
AX_PATH=$(AX_BDIR)/src/libsignal-protocol-c.a

PKG_CONFIG ?= pkg-config
GLIB_CFLAGS ?= $(shell $(PKG_CONFIG) --cflags glib-2.0)
Expand Down Expand Up @@ -46,9 +46,11 @@ PKGCFG_L=$(GLIB_LDFLAGS) \
REQPKG=libsignal-protocol-c
REQPKG:=$(shell $(PKG_CONFIG) --exists $(REQPKG) && echo '$(REQPKG)')
ifneq ($(REQPKG),)
AX_PATH_AS_NEEDED =
PKGCFG_C += $(SIGNAL_CFLAGS)
PKGCFG_L += $(SIGNAL_LDFLAGS)
else
AX_PATH_AS_NEEDED = $(AX_PATH)
HEADERS=-I$(AX_DIR)/src
PKGCFG_C +=$(HEADERS)
PKGCFG_L +=$(AX_PATH)
Expand All @@ -73,20 +75,20 @@ all: $(BDIR)/libaxc.a shared
$(BDIR):
$(MKDIR_P) $@

client: $(SDIR)/message_client.c $(BDIR)/axc_store.o $(BDIR)/axc_crypto.o $(BDIR)/axc.o $(AX_PATH)
client: $(SDIR)/message_client.c $(BDIR)/axc_store.o $(BDIR)/axc_crypto.o $(BDIR)/axc.o $(AX_PATH_AS_NEEDED)
$(MKDIR_P) $@
$(CC) $(CPPFLAGS) $(CFLAGS) $^ -o $@/$@.o $(LDFLAGS)

$(BDIR)/axc.o: $(SDIR)/axc.c $(BDIR)
$(BDIR)/axc.o: $(SDIR)/axc.c $(AX_PATH_AS_NEEDED) | $(BDIR)
$(CC) $(PICFLAGS) $(CPPFLAGS) -c $< -o $@

$(BDIR)/axc-nt.o: $(SDIR)/axc.c $(BDIR)
$(BDIR)/axc-nt.o: $(SDIR)/axc.c $(AX_PATH_AS_NEEDED) | $(BDIR)
$(CC) $(PICFLAGS) $(CPPFLAGS) -DNO_THREADS -c $< -o $@

$(BDIR)/axc_crypto.o: $(SDIR)/axc_crypto.c $(BDIR)
$(BDIR)/axc_crypto.o: $(SDIR)/axc_crypto.c $(AX_PATH_AS_NEEDED) | $(BDIR)
$(CC) $(PICFLAGS) $(CPPFLAGS) -c $< -o $@

$(BDIR)/axc_store.o: $(SDIR)/axc_store.c $(BDIR)
$(BDIR)/axc_store.o: $(SDIR)/axc_store.c $(AX_PATH_AS_NEEDED) | $(BDIR)
$(CC) $(PICFLAGS) $(CPPFLAGS) -c $< -o $@

$(BDIR)/libaxc.a: $(BDIR)/axc.o $(BDIR)/axc_crypto.o $(BDIR)/axc_store.o
Expand All @@ -95,7 +97,7 @@ $(BDIR)/libaxc.a: $(BDIR)/axc.o $(BDIR)/axc_crypto.o $(BDIR)/axc_store.o
$(BDIR)/libaxc-nt.a: $(BDIR)/axc-nt.o $(BDIR)/axc_crypto.o $(BDIR)/axc_store.o
$(AR) rcs $@ $^

$(BDIR)/libaxc.so: $(BDIR)
$(BDIR)/libaxc.so: $(AX_PATH_AS_NEEDED) | $(BDIR)
$(CC) -shared -Wl,-soname,libaxc.so.$(VER_MAJ) -o $@ $(PICFLAGS) $(SDIR)/axc.c $(SDIR)/axc_crypto.c $(SDIR)/axc_store.c $(LDFLAGS) $(CPPFLAGS)

$(BDIR)/libaxc.pc: $(BDIR)
Expand All @@ -110,13 +112,15 @@ $(BDIR)/libaxc.pc: $(BDIR)
echo 'Cflags: -I$${includedir}/axc' >> $@
echo 'Libs: -L$${libdir} -laxc' >> $@

$(AX_PATH):
cd $(AX_DIR) && \
$(MKDIR_P) build && \
cd build && \
$(CMAKE) $(CMAKE_FLAGS) .. && \
$(MAKE)
$(AX_DIR):
@echo "ERROR: Git submodules are not initialized, please run e.g. 'git submodule update --init --recursive' first" >&2 ; false

$(AX_BDIR): | $(AX_DIR)
$(MKDIR_P) $@

$(AX_PATH): | $(AX_BDIR)
cd $(AX_BDIR) && $(CMAKE) $(CMAKE_FLAGS) ..
$(MAKE) -C $(AX_BDIR)

shared: $(BDIR)/libaxc.so $(BDIR)/libaxc.pc

Expand All @@ -134,22 +138,17 @@ install: $(BDIR)
install -m 644 $(SDIR)/axc_store.h $(DESTDIR)/$(PREFIX)/include/axc/


ifneq ($(REQPKG),)
.PHONY: test
test: test_store test_client
else
.PHONY: test
test: $(AX_PATH) test_store test_client
endif

.PHONY: test_store
test_store: $(SDIR)/axc_store.c $(SDIR)/axc_crypto.c $(TDIR)/test_store.c
test_store: $(SDIR)/axc_store.c $(SDIR)/axc_crypto.c $(TDIR)/test_store.c $(AX_PATH_AS_NEEDED)
$(CC) $(TESTFLAGS) -o $(TDIR)/$@.o $(TDIR)/test_store.c $(SDIR)/axc_crypto.c $(LDFLAGS_T)
-$(TDIR)/$@.o
find . -maxdepth 1 -iname 'test*.g*' -exec mv {} $(TDIR) \;

.PHONY: test_client
test_client: $(SDIR)/axc.c $(SDIR)/axc_crypto.c $(SDIR)/axc_store.c $(TDIR)/test_client.c
test_client: $(SDIR)/axc.c $(SDIR)/axc_crypto.c $(SDIR)/axc_store.c $(TDIR)/test_client.c $(AX_PATH_AS_NEEDED)
$(CC) $(TESTFLAGS) -o $(TDIR)/$@.o $(SDIR)/axc_crypto.c $(TDIR)/test_client.c $(LDFLAGS_T)
-$(TDIR)/$@.o
find . -maxdepth 1 -iname 'test*.g*' -exec mv {} $(TDIR) \;
Expand All @@ -168,6 +167,6 @@ clean:

.PHONY: clean-all
clean-all: clean
rm -rf client $(BDIR) $(CDIR) $(AX_DIR)/build
rm -rf client $(BDIR) $(CDIR) $(AX_BDIR)