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

Fixed msys2 build and re-activated CI Mingw-w64 build. #1491

Merged
merged 1 commit into from
Apr 15, 2022
Merged

Fixed msys2 build and re-activated CI Mingw-w64 build. #1491

merged 1 commit into from
Apr 15, 2022

Conversation

utoni
Copy link
Collaborator

@utoni utoni commented Mar 16, 2022 •
edited
Loading

  • Removed Visual Studio leftovers. Maintaining an autotools project with VS integration requires some additional overhead.

Signed-off-by: Toni Uhlig matzeton@googlemail.com

@utoni utoni force-pushed the fix/windows-msys2 branch 2 times, most recently from b5eb4b0 to 43333fb Compare March 16, 2022 10:56
@utoni utoni changed the title Fixed Windows CC build warnings and re-activated CI Mingw64 build. Fixed msys2 build warnings and re-activated CI Mingw64 build. Mar 16, 2022
@utoni utoni force-pushed the fix/windows-msys2 branch 6 times, most recently from c992547 to bd4b728 Compare March 16, 2022 17:37
@aouinizied
Copy link
Collaborator

@lnslbrty amazing work!
Just some remarks based on what I already tested:

  • Keep libgcrypt and libgperror (clone build from source etc.) steps as these are not available targets (didn't succeed to find them at least).
  • Also no need for libpcap as it is already on Windows runners with WinPcap.
  • For rrd, python, pcre and maxmind, I guess we want an initial support on CI with libndpi compiled with and without libgcrypt and tested. Then we can add stuff for these targets. What do you think?

With your fixes you will be able to add testing. Note that it fails if you keep --enable-tls-sigs and I remove it on my local windows build in order to run the following and see pcaps when tests fails.

Hope this helps.

msys2 -c 'cd tests'
msys2 -c 'do.sh'

@utoni
Copy link
Collaborator Author

utoni commented Mar 16, 2022 •
edited
Loading

@lnslbrty amazing work! Just some remarks based on what I already tested:

Thanks! <3

* Keep libgcrypt and libgperror (clone build from source etc.) steps as these are not available targets (didn't succeed to find them at least).

According to msys2 pkgs it is. But I am not sure if you need to add some special repo components/url somewhere. (similiar to debian based distro's e.g. base/contrib/non-free/etc)

* Also no need for libpcap as it is already on Windows runners with WinPcap.

True. Totally forgot about that.

* For rrd, python, pcre and maxmind, I guess we want an initial support on CI with libndpi compiled with and without libgcrypt and tested. Then we can add stuff for these targets. What do you think?

I will comment that rrd and python stuff out. Support for libpcre and libmaxmind was there before.

With your fixes you will be able to add testing. Note that it fails if you keep --enable-tls-sigs and I remove it on my local windows build in order to run the following and see pcaps when tests fails.

Interesting. It shouldn't. Will investigate.

Hope this helps.

msys2 -c 'cd tests'
msys2 -c 'do.sh'

It does indeed. Thanks!

@utoni utoni force-pushed the fix/windows-msys2 branch 14 times, most recently from 53823fc to ad6c7cc Compare March 19, 2022 17:15
@utoni
Copy link
Collaborator Author

utoni commented Mar 21, 2022

@aouinizied
The CI shows some pcap diff's, but looking at those diff's it seems that there aren't any. Is this a classy '\r\n' issue?

@IvanNardi
Copy link
Collaborator

@aouinizied The CI shows some pcap diff's, but looking at those diff's it seems that there aren't any. Is this a classy '\r\n' issue?

I briefly looked at https://github.com/ntop/nDPI/runs/5612568828?check_suite_focus=true and it seems that some diffs are real.
For example:

  • 4in4tunnel.pcap and 4in6tunnel.pcap are clearly different: it seems that the decapsulation code is not triggered
  • 1kxun.pcap flow 1: the URL field is one byte longer: the last bytes were "656" and are "6569" now
  • netflix.pcap flows 10, 13, 16: the URL field is one byte longer

@utoni utoni force-pushed the fix/windows-msys2 branch 2 times, most recently from 5f2a939 to c84b905 Compare March 25, 2022 10:38
@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging ee42a0e into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@utoni utoni force-pushed the fix/windows-msys2 branch 2 times, most recently from 3d2a147 to 5a02b71 Compare April 14, 2022 09:50
@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging 5a02b71 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging 39cf647 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@utoni utoni changed the title Fixed msys2 build warnings and re-activated CI Mingw64 build. Fixed msys2 build and re-activated CI Mingw64 build. Apr 14, 2022
@utoni utoni changed the title Fixed msys2 build and re-activated CI Mingw64 build. Fixed msys2 build and re-activated CI Mingw-w64 build. Apr 14, 2022
@utoni utoni marked this pull request as ready for review April 14, 2022 11:19
@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging 1281d0e into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging fd5a6e0 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging ee68bc8 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging 1e728d3 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging b235046 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging 693bf33 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@utoni
Fixed msys2 build warnings and re-activated CI Mingw64 build.
c3df3a1 
 * Removed Visual Studio leftovers. Maintaining an autotools project with VS integration requires some additional overhead.

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
Signed-off-by: lns <matzeton@googlemail.com>
@sonarcloud
Copy link

sonarcloud bot commented Apr 14, 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
0.0% 0.0% Duplication

@lgtm-com
Copy link

lgtm-com bot commented Apr 14, 2022

This pull request fixes 5 alerts when merging c3df3a1 into 4775be3 - view on LGTM.com

fixed alerts:

  • 4 for Potentially overflowing call to snprintf
  • 1 for Local variable hides global variable

@aouinizied
Copy link
Collaborator

@utoni Impressive work. Thanks for this contribution.

@aouinizied aouinizied merged commit fba75a3 into ntop:dev Apr 15, 2022
@utoni utoni mentioned this pull request May 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanNardi IvanNardi IvanNardi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@utoni @aouinizied @IvanNardi