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.

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

Improve CI #1303

Merged
merged 23 commits into from
Sep 22, 2021
Merged

Improve CI #1303

merged 23 commits into from
Sep 22, 2021

Conversation

aouinizied
Copy link
Collaborator

This PR improves the current CI pipeline:

  • Add macOS platform (both 11 and 10.15).
  • Add Ubuntu 18.04 target.
  • Add libgcrypt target (with and without on all targets).
  • Keep building for ARM and mingw only on ubuntu latest target.

This PR addresses previously discussion in: #1289

Zied

@aouinizied
Copy link
Collaborator Author

@lnslbrty note that my changes do not affect other builds etc.
Here are some ideas where feedback is welcome:

  • I can move fuzzing to this CI script too and then make all in one CI.
  • I can set some GCC and CLANG versions as part of the matrix (maybe only on ubuntu target?).
  • Should we definitively drop Travis?
  • How about building on a Windows target too? (Note that I'm not an expert on building stuff on Windows and that if it looks ok for you I will need some help)

@IvanNardi
Copy link
Collaborator

IvanNardi commented Sep 13, 2021

@aouinizied : I am not sure what's going on, but CI didn't run on your PR...
Am I missing something? Is that expected?

@aouinizied
Copy link
Collaborator Author

@IvanNardi I think this is normal as it's a PR changing the CI file.
Already tested it using an nDPI like repo here https://github.com/nfstream/nDPI-actions/actions
I already made the changes (remove nDPI repo cloning etc) before opening the PR.

@utoni
Copy link
Collaborator

utoni commented Sep 14, 2021

@lnslbrty note that my changes do not affect other builds etc.
Here are some ideas where feedback is welcome:

* I can move fuzzing to this CI script too and then make all in one CI.

* I can set some GCC and CLANG versions as part of the matrix (maybe only on ubuntu target?).

* Should we definitively drop Travis?

* How about building on a Windows target too? (Note that I'm not an expert on building stuff on Windows and that if it looks ok for you I will need some help)
  1. Unsure if we should move fuzzing to the CI script. Are there any benefits?
  2. It would be probably worth to build nDPI at least twice with the lowest and highest supported CC.
  3. Yeah, I would like to see the nDPI repository without any leftover files. We still have a GIT history. And with a meaningful commit message, we can always revert that commit.
  4. I would advise to build at least with Mingw-w64. Building with Visual Studio won't probably as easy and may cause more maintaining work.

@utoni
Copy link
Collaborator

utoni commented Sep 14, 2021

I do not see any arm or mingw builds. What happened?

// EDIT: The first two jobs build for mingw.

Is it possible to rename jobs? instead of ubuntu-* DISABLE_GCRYPT something like ubuntu-* DISABLE_GCRYPT arm mingw?

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@aouinizied
Copy link
Collaborator Author

aouinizied commented Sep 14, 2021

I do not see any arm or mingw builds. What happened?

// EDIT: The first two jobs build for mingw.

Is it possible to rename jobs? instead of ubuntu-* DISABLE_GCRYPT something like ubuntu-* DISABLE_GCRYPT arm mingw?

Name generation will be tricky. However I changed the script. Now, cross compilation is triggered for all ubuntu jobs and I added a notice to the step name displayed to make it more clear.

EDIT: Now reverted to ubuntu-latest only as compilation with mingw fails on ubuntu 18.04 with
/usr/bin/x86_64-w64-mingw32-ld: cannot find -lucrtbase

@aouinizied
Copy link
Collaborator Author

@lnslbrty Travis dropped and badge icon replaced.

@utoni
Copy link
Collaborator

utoni commented Sep 14, 2021

I do not see any arm or mingw builds. What happened?
// EDIT: The first two jobs build for mingw.
Is it possible to rename jobs? instead of ubuntu-* DISABLE_GCRYPT something like ubuntu-* DISABLE_GCRYPT arm mingw?

Name generation will be tricky. However I changed the script. Now, cross compilation is triggered for all ubuntu jobs and I added a notice to the step name displayed to make it more clear.

EDIT: Now reverted to ubuntu-latest only as compilation with mingw fails on ubuntu 18.04 with
/usr/bin/x86_64-w64-mingw32-ld: cannot find -lucrtbase

Explicit linkage of -lucrtbase was added by yourself in 812a3e7. But I do not know why it should be necessary. As far as I can remember it is used for development/debugging purposes. Maybe you can try if it works for both by just removing -lucrtbase in configure.seed.

@aouinizied
Copy link
Collaborator Author

I do not see any arm or mingw builds. What happened?
// EDIT: The first two jobs build for mingw.
Is it possible to rename jobs? instead of ubuntu-* DISABLE_GCRYPT something like ubuntu-* DISABLE_GCRYPT arm mingw?

Name generation will be tricky. However I changed the script. Now, cross compilation is triggered for all ubuntu jobs and I added a notice to the step name displayed to make it more clear.
EDIT: Now reverted to ubuntu-latest only as compilation with mingw fails on ubuntu 18.04 with
/usr/bin/x86_64-w64-mingw32-ld: cannot find -lucrtbase

Explicit linkage of -lucrtbase was added by yourself in 812a3e7. But I do not know why it should be necessary. As far as I can remember it is used for development/debugging purposes. Maybe you can try if it works for both by just removing -lucrtbase in configure.seed.

@lnslbrty it was probably a dummy fix for some windows CI I was trying to setup. Removed it and now it works on both. Thanks for your help.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@IvanNardi
Copy link
Collaborator

Is it possible to test multiple compilers, as suggested earlier?
No strong opinions but something like gcc-11, gcc-7, clang-12 and clang-7 on ubuntu-latest only... not sure about the versions: the idea is to test the newest compilers and something "old" (but easily available on the target)
In the old Travis configuration there was something like that.
Just an idea

@utoni
Copy link
Collaborator

utoni commented Sep 16, 2021

Is it possible to test multiple compilers, as suggested earlier?
No strong opinions but something like gcc-11, gcc-7, clang-12 and clang-7 on ubuntu-latest only... not sure about the versions: the idea is to test the newest compilers and something "old" (but easily available on the target)
In the old Travis configuration there was something like that.
Just an idea

I would like to see libnDPI compiling/testing on a bigendian architecture e.g. armhf.
The Debian project reported some issues on bigendian machines.

While compiling for armhf -mbig-endian is pretty easy, testing might be not as trivial. So this is just a "nice-to-have" (may be also part of a follow-up PR).

@aouinizied
Copy link
Collaborator Author

@IvanNardi Yes, need to set it up. But we must implement at least 2 versions on each target for 2 compilers. I will rather make it part of the matrix. It will result in 24 jobs (8 current jobs * 4 versions (2 of CLANG and 2 of GCC let's say)) but this is not an issue as GitHub actions are limited to 255 jobs per matrix (AFAIK).

@lnslbrty We will have mainly two options if you want to build and test on arm using Github Actions:

Zied

@utoni
Copy link
Collaborator

utoni commented Sep 16, 2021

@IvanNardi Yes, need to set it up. But we must implement at least 2 versions on each target for 2 compilers. I will rather make it part of the matrix. It will result in 24 jobs (8 current jobs * 4 versions (2 of CLANG and 2 of GCC let's say)) but this is not an issue as GitHub actions are limited to 255 jobs per matrix (AFAIK).

@lnslbrty We will have mainly two options if you want to build and test on arm using Github Actions:

* Have an RPI somewhere (do not need a static IP or whatever), install on it Github actions, and declare it within the ntop organization as a self-hosted runner. I have some RPIs left somewhere and I was thinking about such an option for nfstream project (to publish arm wheels). But as always, need to stop procrastinating and give it a try soon.
  Examples: https://dev.to/l04db4l4nc3r/self-hosted-github-actions-using-raspberry-pi-212m

* The second one will be to use QEMU to emulate such architectures and this can be set up on ubuntu-latest target for example.
  Idea from: https://github.community/t/testing-against-multiple-architectures/17111/9

Zied

While the first option sounds interesting and I will probably try it out for my own projects, for libnDPI I would stick to a "all-in-one" solution that does not depend on self-hosted machines. I am unsure if the performance drop for the second option is huge. My experience with qemu-arm is that it is somewhat slow compared to VM's with a hypervisor available. But I would still prefer option #2.

@utoni
Copy link
Collaborator

utoni commented Sep 16, 2021

@aouinizied There are also some (not serious) Code Inspector complaints. It is all about indentation and unnecessary whitespaces, but should be easy to fix.

@aouinizied
Copy link
Collaborator Author

aouinizied commented Sep 21, 2021

@IvanNardi @lnslbrty I added jobs on Ubuntu Latest using the following old and new versions logic:

  • GCC: 7 and 10
  • CLANG: 7 and 12

We continue to build on ubuntu-latest, ubuntu 18.04, macos11 and macos10.15 using the default compiler of the runner (gcc for ubuntu and clang for MacOS)

I fixed the CodeInspector complaints too.

As now we have improved our CI on Github Actions, Travis is definitively dropped.

ARM target using Qemu and build on Windows server using MinGW will be part of a follow PR as I need to investigate several issues. Consequently, this PR can be merged as it is.

Zied

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@utoni utoni left a comment

Choose a reason for hiding this comment

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

Good job!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@aouinizied
Copy link
Collaborator Author

@lnslbrty Thanks for your feedbacks.

  • I fixed make all to take into account python wrapper and ndpiSimpleIntegration
  • I fixed broken RCE detection within (ndpi_utils).
  • Now tests take into account PCRE flag for WebattackRCE pcap. By default the result is the one with libpcre and when libpcre is not set, pcap is skipped (same logic done with gcrypt). [configure.seed + do.sh + pcap result file]
  • I added jobs that enable: --with-sanitizer --with-pcre --with-maxminddb on all platform + each clang/gcc specific version job.

Here are some notes:

  • --with-sanitizer is broken on macOS (Apple LLVM CLANG) and MinGW.

Zied

@sonarcloud
Copy link

sonarcloud bot commented Sep 22, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@aouinizied
Copy link
Collaborator Author

@lnslbrty also dropped appveyor.yml because it is not relevant anymore.

Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

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

Great and very useful wok. Thanks!

@utoni utoni merged commit 0994771 into ntop:dev Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants