Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Windows-msys2 workflows #1666

maxirmx · 2021-11-01T21:06:57Z

maxirmx · 2021-11-01T21:11:17Z

@ronaldtse
I added Clang builds to Ubuntu jobs and it works.
I would say that it is expected behavior since Clangs team maintains strong compatibility with gcc.
Is that what we are looking for or I misunderstand the task ?

ronaldtse · 2021-11-02T06:41:42Z

@maxirmx yes it should pass as we know that Thunderbird is using this configuration.

@kaie whom should we seek if we want to know what CI configurations we should test to ensure builds at TB work? Thanks!

ni4

I'd suggest to remove redundant tests, where we (most likely) do not need to check different compilers.

.github/workflows/ubuntu.yml

Clang build and test for the following workflows: - centos7 - centos8-ossl - centos8 - debian9 - macos - ubuntu Fixed: - #1670 - #1669 - #1668 - Packaging tests for centos8 (gcc and clang)

maxirmx · 2021-11-06T06:44:04Z

Clang build and test for the following workflows:

centos7
centos8-ossl
centos8
debian9
macos
ubuntu
windows (msys)

Fixed:

clang/rnp build issue (CentOS 7 workflow) #1670
clang/GnuPG v2.3.0-beta983 issue (CentOS 8 workflow) #1669
clang/libgcrypt v1.9.3 issue (CentOS 8 workflow) #1668
Broken legacy packaging tests for centos8
Several legacy minor issues flagged by lint/shellcheck in ci scripts

* Added clang to msys workflow * Removed empty lines * Commented out test we think we do not need

* Added clang build\test to Msys workflow * Fixed ci scripts issues reported by lint

maxirmx · 2021-11-07T21:17:01Z

@ronaldtse @ni4

I believe this PR is ready for merge. All changes are listed in the previous comment:#1666 (comment)

Some general considerations:

This PR is about changes to YAML and CI shell scripts so that they could generate correct clang configurations. I reduced the number of tests as @ni4 asked in his review but I have some doubts that such reduction is consistent.
CI shell scripts are very difficult to understand, modify and maintain. I believe they need refactoring and using package manager (or managers) as use vcpkg for all platforms #1648 suggests would be a good approach. Investigate streamlining CI processes to reduce time on cross-platform debugging #1508 will also work but it will force using specific (possibly outdated) versions of dependencies.

ronaldtse · 2021-11-08T01:53:00Z

CI shell scripts are very difficult to understand, modify and maintain. I believe they need refactoring and using package manager (or managers) as use vcpkg for all platforms #1648 suggests would be a good approach. Investigate streamlining CI processes to reduce time on cross-platform debugging #1508 will also work but it will force using specific (possibly outdated) versions of dependencies.

I agree. Let's merge this first and migrate to a package manager?

ronaldtse

Thank you @maxirmx !

.github/workflows/ubuntu.yml

.github/workflows/windows.yml

ronaldtse · 2021-11-07T13:58:33Z

ci/lib/install_functions.inc.sh

+   libtool
+   bzip2
+   gzip
+ )


Extra preceding whitespace for these lines?

I do not agree. I made identation consistent. Without extra whitespace these 4 lines have identation that differs from the code before and after.

Those seem to have 2-space indentation before the changes, or do I miss something?

Yes it had double spaces before the changes and it has double spaces after changes.
I think there was a version in the middle that had wrong spacing.

I think the version here is the version that has wrong spacing :)

I agree with everything you say about spacing and empty lines.

ronaldtse · 2021-11-09T01:52:54Z

Ping @ni4 @ribose-jeffreylau for one more review.

ronaldtse · 2021-11-09T06:18:54Z

@maxirmx it's okay to have this merged right now since it is only CI related.

maxirmx · 2021-11-09T06:32:22Z

@ronaldtse
It now has a conflict with #1681
Sorry, I wanted to merge the opposite direction

ribose-jeffreylau

@maxirmx I left some comments above. Thanks!

ribose-jeffreylau · 2021-11-09T06:17:56Z

ci/env-msys.inc.sh

-export CC=gcc
-export CXX=g++
+export CC=${CC-gcc} 
+export CXX=${CXX-g++}


If $CC (and $CXX) are set to empty strings, after CC=${CC-gcc}, $CC would still remain as empty strings. Is this intended? (similar with $CXX)

Also, there is a trailing white space on line 9.

This is protection against CC/CXX unset.
CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

Actually it is an interesting issue related to CI logic consistency.
CI logic is distributed accross YAML-shell scripts-cmake If thess shell scripts are always called from GHA you can ensure that CC and other variablle are set in YAML. Would you like to rely on this scenario only or hacve some extra logic that make scripts work reliably without GHA ?

I see, I didn't know about cmake's behaviour on empty string CC :) Thanks!

Before your reply, I would have preferred export CC=${CC:-gcc} with the extra colon, so CC would actually explicitly take on the value of "gcc" instead of becoming an empty string when CC is originally unset.

Observe:

$ unset TEST $ echo "${TEST-this is a test}" this is a test $ TEST= $ echo "${TEST-this is a test}" $ echo "${TEST:-this is a test}" this is a test

Actually it is an interesting issue related to CI logic consistency.
CI logic is distributed accross YAML-shell scripts-cmake If thess shell scripts are always called from GHA you can ensure that CC and other variablle are set in YAML. Would you like to rely on this scenario only or hacve some extra logic that make scripts work reliably without GHA ?

For now, I think we can safely assume that these scripts are always run within GHA, as long as they live inside ci/.

Actually I also put export CC=${CC:-gcc} first but then I relized that if empty string comes from the upstream it does not hurt but may have some idea behind it.

ribose-jeffreylau · 2021-11-09T06:23:35Z

ci/lib/install_functions.inc.sh

+   libtool
+   bzip2
+   gzip
+ )


I think the version here is the version that has wrong spacing :)

ribose-jeffreylau · 2021-11-09T06:24:52Z

ci/lib/install_functions.inc.sh

@@ -522,6 +526,17 @@ linux_install_debian() {
    "${build_dependencies_deb[@]}" \
    "$@"

+  if [ "${CC-gcc}" = "clang" ]; then


Same issue as above --- with $CC set to empty string, "${CC-gcc}" would still be empty string. Is this intended?

CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

ribose-jeffreylau · 2021-11-09T06:25:12Z

ci/lib/install_functions.inc.sh

    mingw64/mingw-w64-x86_64-python3
  )

+  if [ "${CC-gcc}" = "gcc" ]; then


CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

ribose-jeffreylau · 2021-11-09T06:25:19Z

ci/lib/install_functions.inc.sh

  pacman --noconfirm -S --needed "${packages[@]}"

+  if [ "${CC-gcc}" = "gcc" ]; then


CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

ribose-jeffreylau · 2021-11-09T06:25:33Z

ci/lib/install_functions.inc.sh

@@ -833,7 +866,7 @@ build_example_pkgconfig() {
  pushd "$(mktemp -d)" || return 1

  # shellcheck disable=SC2046
-  gcc "${rnpsrc}/src/examples/generate.c" -ogenerate $(pkg-config --cflags --libs $pkgflags librnp) $gccflags
+  ${CC-gcc} "${rnpsrc}/src/examples/generate.c" -ogenerate $(pkg-config --cflags --libs $pkgflags librnp) $gccflags


CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

Here, when CC is an empty string, ${CC-gcc} would actually remain an empty string. See my comment above.

ribose-jeffreylau · 2021-11-09T06:25:45Z

ci/main.sh

    export PATH="${LOCAL_BUILDS}/rnp-build/lib:${LOCAL_BUILDS}/rnp-build/bin:${LOCAL_BUILDS}/rnp-build/src/lib:$PATH"
+    if [[ "${CC-gcc}" = "clang" ]]; then


CC="" will force cmake default which will be gcc. If CC is unset it will also force cmake default but it may fail at this line depending on shell settings.

ribose-jeffreylau · 2021-11-09T06:30:30Z

ci/main.sh

@@ -32,11 +32,22 @@ prepare_build_prerequisites() {
 prepare_test_env() {
  prepare_build_tool_env

-  export LD_LIBRARY_PATH="${GPG_INSTALL}/lib:${BOTAN_INSTALL}/lib:${JSONC_INSTALL}/lib:${RNP_INSTALL}/lib:$LD_LIBRARY_PATH"
+  export LD_LIBRARY_PATH="${GPG_INSTALL}/lib:${BOTAN_INSTALL}/lib:${JSONC_INSTALL}/lib:${RNP_INSTALL}/lib:${LD_LIBRARY_PATH-}"


Would an improvement be:

export ...="...:${RNP_INSTALL}/lib${LD_LIBRARY_PATH:+:${LD_LIBRARY_PATH}}"

so that if $LD_LIBRARY_PATH is an empty string, there would not be a trailing colon?

If $LD_LIBRARY_PATH is unset the script won't fail

If $LD_LIBRARY_PATH is unset the script won't fail

In which case $LD_LIBRARY_PATH would end up having a trailing : at the end. I'm not familiar with the behaviour of $LD_LIBRARY_PATH with empty components, but if it's what's commonly accepted then I'm fine with it :)

ribose-jeffreylau · 2021-11-09T06:31:05Z

ci/lib/install_functions.inc.sh

    git
    automake
    autoconf
    libtool
    automake-wrapper
    gnupg2
    make
-    pkg-config
+    pkg-config 


There's a trailing white space here.

ribose-jeffreylau · 2021-11-09T06:34:34Z

.github/workflows/ubuntu.yml

+       - uses: actions/checkout@v2
+         with:
+           fetch-depth: 1
+       - name: Setup environment
+         run: |
+           . ci/gha/setup-env.inc.sh
+           ci/install_noncacheable_dependencies.sh
+       - name: Cache
+         id: cache
+         uses: actions/cache@v2
+         with:
+           path: ${{ env.CACHE_DIR }}
+           key: ${{ github.workflow }}-${{ github.job }}-${{ runner.os }}-${{ env.BUILD_MODE }}-gpg-${{ env.GPG_VERSION }}-${{ hashFiles('ci/**') }}-${{ hashFiles('.github/workflows/**') }}
+       - name: Build cache
+         if: steps.cache.outputs.cache-hit != 'true'
+         run: |
+           set -x
+           ci/install_cacheable_dependencies.sh
+       - name: tests
+         run: |
+           set -x
+           ci/run.sh
+           ls -la "${RNP_INSTALL}/lib/"librnp*.so
+           ! ctest -N | grep 'ruby' > /dev/null
+           ! ls -la src/tests/ | grep 'ruby' > /dev/null


I think these lines have been indented by 11 white spaces? Maybe an accident?

ni4 · 2021-11-09T10:23:18Z

@maxirmx some other side notes:

we got used to the branch naming like maxirmx-1129-build-with-clang-cl
we also prefer to rebase on master instead of merge, squashing together commits like 'fix of fix', 'fix pr comment'. Interactive rebase makes this easy and straightforward.

maxirmx · 2021-11-09T19:30:34Z

we got used to the branch naming like maxirmx-1129-build-with-clang-cl

Does 'ni4-fix-rnp-hash-destructutor' follow this rule ?

ronaldtse · 2021-11-10T07:52:37Z

@maxirmx we usually use {username}-{topic} as a development branch name under the rnp repo when it is shared. If the PR comes from a forked repository, the branch name isn't important.

ni4 · 2021-11-10T09:55:29Z

we got used to the branch naming like maxirmx-1129-build-with-clang-cl

Does 'ni4-fix-rnp-hash-destructutor' follow this rule ?

In this case there were no issue created for, so, yeah, no issue number.

maxirmx added 7 commits November 1, 2021 20:40

CC, CXX env variables

7a6217c

Update ubuntu.yml

fde70fd

Build matrix

ebec6f6

Update ubuntu.yml

4d64bcc

Matricies for all relevant jobs

7f47817

Update ubuntu.yml

54e5852

Update ubuntu.yml

3e04175

maxirmx requested review from dewyatt and ronaldtse November 1, 2021 21:09

ronaldtse requested a review from ni4 November 2, 2021 06:41

ni4 requested changes Nov 5, 2021

View reviewed changes

.github/workflows/ubuntu.yml Outdated Show resolved Hide resolved

.github/workflows/ubuntu.yml Outdated Show resolved Hide resolved

.github/workflows/ubuntu.yml Outdated Show resolved Hide resolved

.github/workflows/ubuntu.yml Outdated Show resolved Hide resolved

maxirmx added 2 commits November 6, 2021 09:08

Clang build and test (#1)

36109fe

Clang build and test for the following workflows: - centos7 - centos8-ossl - centos8 - debian9 - macos - ubuntu Fixed: - #1670 - #1669 - #1668 - Packaging tests for centos8 (gcc and clang)

Remove empty lines

7d646ce

maxirmx changed the title ~~Added Clang builds to Ubuntu jobs~~ Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 jobs Nov 6, 2021

maxirmx changed the title ~~Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 jobs~~ Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 workflows Nov 6, 2021

maxirmx added 4 commits November 6, 2021 09:48

Merge branch 'rnpgp:master' into master

960a22d

Merge branch 'rnpgp:master' into master

c56d7d1

Added clang to msys workflow (#2)

dac1e4b

* Added clang to msys workflow * Removed empty lines * Commented out test we think we do not need

Add clang build\test to Msys workflow (#4)

c2fe818

* Added clang build\test to Msys workflow * Fixed ci scripts issues reported by lint

maxirmx changed the title ~~Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 workflows~~ Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Woindows-msys2 workflows Nov 7, 2021

maxirmx changed the title ~~Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Woindows-msys2 workflows~~ Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Windows-msys2 workflows Nov 7, 2021

maxirmx requested a review from ni4 November 7, 2021 21:17

This was linked to issues Nov 7, 2021

ci: Build with clang-cl #1129

Closed

clang/libgcrypt v1.9.3 issue (CentOS 8 workflow) #1668

Closed

clang/GnuPG v2.3.0-beta983 issue (CentOS 8 workflow) #1669

Closed

clang/rnp build issue (CentOS 7 workflow) #1670

Closed

ronaldtse approved these changes Nov 8, 2021

View reviewed changes

Removed extra whitespaces

6faf51b

maxirmx mentioned this pull request Nov 8, 2021

Disable SC2295. #1681

Merged

maxirmx referenced this pull request Nov 8, 2021

Fix few CI shellcheck errors.

dcdfa63

Removed extra whitespaces

4410c86

maxirmx merged commit 4ca4400 into rnpgp:master Nov 9, 2021

maxirmx mentioned this pull request Nov 9, 2021

Revert "Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Windows-msys2 workflows" #1682

Merged

ribose-jeffreylau requested changes Nov 9, 2021

View reviewed changes

This was referenced Nov 10, 2021

Fix centos8/pkgconfig-cmake-target job that is disabled in CI #1685

Closed

Functionality that helps run/debug CI scripts locally (outside GHA environment) #1686

Closed

Clean up history to remove failed merge of #1666 and #1682 #1687

Closed

ronaldtse mentioned this pull request Nov 11, 2021

Add clang builds: Ubuntu, CentOS 7/8 (+ ossl), macOS, Debian9 and Windows-msys2 (from #1684) #1688

Merged

ni4 added this to the v0.16.0 milestone Jan 13, 2022

		pacman --noconfirm -S --needed "${packages[@]}"

		if [ "${CC-gcc}" = "gcc" ]; then

		export PATH="${LOCAL_BUILDS}/rnp-build/lib:${LOCAL_BUILDS}/rnp-build/bin:${LOCAL_BUILDS}/rnp-build/src/lib:$PATH"
		if [[ "${CC-gcc}" = "clang" ]]; then

Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Windows-msys2 workflows #1666

Added Clang builds to Ubuntu, CentOS7, CentOS8 (+ ossl), MacOS, Debian9 and Windows-msys2 workflows #1666

Conversation

maxirmx commented Nov 1, 2021

maxirmx commented Nov 1, 2021

ronaldtse commented Nov 2, 2021

ni4 left a comment

Choose a reason for hiding this comment

maxirmx commented Nov 6, 2021 • edited Loading

maxirmx commented Nov 7, 2021

ronaldtse commented Nov 8, 2021

ronaldtse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronaldtse commented Nov 9, 2021

ronaldtse commented Nov 9, 2021

maxirmx commented Nov 9, 2021

ribose-jeffreylau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxirmx Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni4 commented Nov 9, 2021

maxirmx commented Nov 9, 2021 • edited Loading

ronaldtse commented Nov 10, 2021

ni4 commented Nov 10, 2021

maxirmx commented Nov 6, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx Nov 9, 2021 •

edited

Loading

maxirmx commented Nov 9, 2021 •

edited

Loading