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

Fix eCAP header includes #1753

Closed
wants to merge 2 commits into from
Closed

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Mar 23, 2024

Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.

Squid style guidelines require .h files to be
wrapped with HAVE_*_H protection.

Add the missing ./configure header checks to
generate the needed wrappers and refactor the
include sequences to meet current guidelines.
Comment on lines +910 to +911
/* libecap-1.0.1 headers do not build without autoconf.h magic */
#define HAVE_CONFIG_H
Copy link
Contributor Author

Choose a reason for hiding this comment

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

libecap apparently lacks basic self-consistency checks for its installed headers.
AC_CHECK_HEADERS exposes

/usr/include/libecap/common/registry.h:15:75: error: 'LIBECAP_VERSION' was not declared in this scope
   15 | extern bool RegisterVersionedService(adapter::Service *s, const char *v = LIBECAP_VERSION);

],,,[
/* libecap-1.0.1 headers do not build without autoconf.h magic */
#define HAVE_CONFIG_H
/* libecap/common/delay.h fails to include <string> */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

libecap apparently lacks basic self-consistency checks for its installed headers.
AC_CHECK_HEADERS exposes a number of identical std::string errors, one example:

/usr/include/libecap/common/delay.h:15:43: error: 'string' in namespace 'std' does not name a type
   15 |                 explicit Delay(const std::string &state_):
      |                                           ^~~~~~
/usr/include/libecap/common/delay.h:7:1: note: 'std::string' is defined in header '<string>'; this is probably fixable by adding '#include <string>'

@kinkie
Copy link
Contributor

kinkie commented Mar 23, 2024

May I assume that after this PR, build issues are solved?

@yadij
Copy link
Contributor Author

yadij commented Mar 23, 2024

Yes the commented lines are all that is necessary. These issues do no appear during the main build because we are an autoconf project and other files include <string>. They only show up when ./configure checks for the headers usability.

@yadij yadij closed this Mar 23, 2024
@yadij yadij reopened this Mar 23, 2024
@rousskov
Copy link
Contributor

May I assume that after this PR, build issues are solved?

What build issues? PR description does not mention any fixed build issues, and the PR has only one commit. What am I missing?

@yadij
Copy link
Contributor Author

yadij commented Mar 24, 2024

May I assume that after this PR, build issues are solved?

What build issues? PR description does not mention any fixed build issues, and the PR has only one commit. What am I missing?

The code comments documenting why the AC_CHECK_HEADER hacks are present.

@rousskov
Copy link
Contributor

May I assume that after this PR, build issues are solved?

What build issues? PR description does not mention any fixed build issues, and the PR has only one commit. What am I missing?

The code comments documenting why the AC_CHECK_HEADER hacks are present.

I was aware of those comments when I asked the question. I am interpreting your response as "The 'build issues' Francesco is talking about are not present in master/v7 code". Please correct me if I am wrong.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 24, 2024
squid-anubis pushed a commit that referenced this pull request Apr 2, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 2, 2024
squid-anubis pushed a commit that referenced this pull request Apr 2, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 2, 2024
squid-anubis pushed a commit that referenced this pull request Apr 3, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 3, 2024
squid-anubis pushed a commit that referenced this pull request Apr 4, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 4, 2024
@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-author author action is expected (and usually required) labels Apr 5, 2024
@squid-anubis squid-anubis added M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 5, 2024
@yadij
Copy link
Contributor Author

yadij commented Apr 5, 2024

Marking for merge under the 10+ day criteria.

@squid-anubis squid-anubis removed the M-abandoned-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Apr 5, 2024
squid-anubis pushed a commit that referenced this pull request Apr 5, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Apr 5, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Apr 5, 2024
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 9, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 9, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
@yadij yadij deleted the arc-autoconf-ng-38 branch April 10, 2024 11:31
kinkie pushed a commit to kinkie/squid that referenced this pull request Apr 14, 2024
Squid style guidelines require .h files to be wrapped with
HAVE_*_H protection and placed after all Squid internal file
includes.

Add the missing ./configure header checks to generate the
needed wrappers and refactor the include sequences to meet
current guidelines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
4 participants