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

configure: remove iconv check #9151

Closed

Conversation

catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Jul 6, 2023

Link to redmine ticket:
None, generic cleaning
Actually https://redmine.openinfosecfoundation.org/issues/4083

Describe changes:

  • configure: remove iconv check

libhtp's configure does the work

As it is not used by Suricata and libhtp does check for it itself
@catenacyber catenacyber requested a review from a team as a code owner July 6, 2023 08:24
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #9151 (fdbd4ba) into master (9b09b29) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9151      +/-   ##
==========================================
- Coverage   82.34%   82.33%   -0.02%     
==========================================
  Files         968      968              
  Lines      273546   273546              
==========================================
- Hits       225247   225219      -28     
- Misses      48299    48327      +28     
Flag Coverage Δ
fuzzcorpus 64.58% <ø> (-0.02%) ⬇️
suricata-verify 60.72% <ø> (-0.03%) ⬇️
unittests 62.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jufajardini
Copy link
Contributor

Who'd be a good reviewer for this?

@catenacyber
Copy link
Contributor Author

You can do it : it simply removes 2 lines, and you can check that iconv is not used by Suricata itself

@jufajardini
Copy link
Contributor

You can do it : it simply removes 2 lines, and you can check that iconv is not used by Suricata itself

Fair XD

Only seems to be used by Doxygen: https://github.com/OISF/suricata/blob/master/doxygen.cfg#L22
I don't know how our Doxygen documentation is generated. 🤔 could it be that it is there for the process of automatically generating those?

@catenacyber
Copy link
Contributor Author

I would argue Suricata does not need iconv even if doxygen needs it

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

My local checks seem to indicate that this change can go :)

@victorjulien
Copy link
Member

Originally added to fix build on freebsd, see c099349

@victorjulien
Copy link
Member

Triggered a run on my qa to see how modern freebsd and openbsd like this change.

@victorjulien
Copy link
Member

FreeBSD 12 for this PR (rebased to master):

  AR       libsuricata_c.a
  CCLD     suricata
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv_open [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconvctl [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv_close [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv [--no-allow-shlib-undefined]
cc: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[3]: *** [Makefile:2857: suricata] Error 1
gmake[3]: Leaving directory '/var/tmp/gitlab_runner/builds/sP2xB2Af/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub/src'
gmake[2]: *** [Makefile:2682: all] Error 2
gmake[2]: Leaving directory '/var/tmp/gitlab_runner/builds/sP2xB2Af/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub/src'
gmake[1]: *** [Makefile:491: all-recursive] Error 1
gmake[1]: Leaving directory '/var/tmp/gitlab_runner/builds/sP2xB2Af/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub'
gmake: *** [Makefile:705: distcheck] Error 1

FreeBSD 13:

  AR       libsuricata_c.a
  CCLD     suricata
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv_open [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconvctl [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv_close [--no-allow-shlib-undefined]
ld: error: ../libhtp/htp/.libs/libhtp.so: undefined reference to libiconv [--no-allow-shlib-undefined]
cc: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[3]: *** [Makefile:2857: suricata] Error 1
gmake[3]: Leaving directory '/var/tmp/gitlab_runner/builds/sF87JEwd/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub/src'
gmake[2]: *** [Makefile:2682: all] Error 2
gmake[2]: Leaving directory '/var/tmp/gitlab_runner/builds/sF87JEwd/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub/src'
gmake[1]: *** [Makefile:491: all-recursive] Error 1
gmake[1]: Leaving directory '/var/tmp/gitlab_runner/builds/sF87JEwd/0/inliniac/suricata-ci/suricata/suricata-7.0.0-rc3-dev/_build/sub'
gmake: *** [Makefile:705: distcheck] Error 1

@catenacyber
Copy link
Contributor Author

ok @cccs-rtmorti would you add this 2-lines removal to your libhtp-rs PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants