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

[WIP] Implement CI on Windows. #1483

Merged
merged 11 commits into from
Mar 9, 2022
Merged

[WIP] Implement CI on Windows. #1483

merged 11 commits into from
Mar 9, 2022

Conversation

aouinizied
Copy link
Collaborator

Now that all issues on big endian were solved by talented people and that we finally saw this green light near s390 line, we can move to new issues and a new red line.
This PR is marked as WIP as I need several attempts to make it work but it will add build and test process on Windows server.
I already did that within nfstream project and saw several issues related to new lightweight encryption library an so on.

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

@lnslbrty now It compiles for both libgcrypt and internal one.
There is a lot of warnings for both of them. We have two options:

  • Treat warning as errors and once all warnings during compilation step are treated, move to test.
  • Enable testing, and then (it will fail for sure) start debugging.

Note also that ndpiSimpleIntegration compilation step is disabled as it fails too, rrd stuff too.

@aouinizied
Copy link
Collaborator Author

@IvanNardi @lnslbrty what do you think is the best option?
Maybe we should merge in the actual state and then people can enable/disable according to the progress.

@utoni
Copy link
Collaborator

utoni commented Mar 8, 2022

  1. merge it right away
  2. merge it, but let the job fail until some takes pity to solve the problem
  3. Is it possible to let the job neither succeed nor fail? Like a warning or so? In real life there are always different shades of grey between black and white. CI's should be like that.

@@ -195,7 +221,15 @@ jobs:
- name: Configure nDPI on MacOS
if: startsWith(matrix.os, 'macOS') && startsWith(matrix.arch, 'x86_64') && startsWith(matrix.compiler, 'default-cc')
run: |
env CC=clang CFLAGS='-Werror' ./autogen.sh --enable-option-checking=fatal --enable-debug-messages ${{ matrix.gcrypt }} ${{ matrix.msan }} ${{ matrix.pcre }} ${{ matrix.maxminddb }} --enable-tls-sigs
env CC=clang CFLAGS='-Werror' ./autogen.sh --enable-option-checking=fatal --enable-debug-messages ${{ matrix.gcrypt }} ${{ matrix.msan }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change on MacOS?

@IvanNardi
Copy link
Collaborator

  • Enable testing, and then (it will fail for sure) start debugging.

Just to be sure I fully understand the current situation: ndpi compiles on Windows (with lots of warnings) but the executable "doesn't work": does that mean that some unit tests fail or that it always crashes?

@sonarcloud
Copy link

sonarcloud bot commented Mar 9, 2022

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 We can merge it as it is. There is warnings but build is passing.

@IvanNardi build pass, but tests do not run (diff).
Based on what I observed on nfstream project these pcaps test fails on Windows:

  • ocs.pcap
  • quic-mvfst-22_decryption_error.pcap
  • http-crash-content-disposition.pcap

@IvanNardi
Copy link
Collaborator

@IvanNardi build pass, but tests do not run (diff). Based on what I observed on nfstream project these pcaps test fails on Windows:

* ocs.pcap

* quic-mvfst-22_decryption_error.pcap

* http-crash-content-disposition.pcap

Thanks for the clarification. Could you share a link with the logs, please?

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.

I think we can merge this PR in the current state and we can fix the issues step-by-step later. Exactly like we did for the big-endian stuff

@utoni utoni merged commit 74ae315 into ntop:dev Mar 9, 2022
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.

3 participants