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

Fix cross-compilation on Gentoo #1920

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix cross-compilation on Gentoo #1920

wants to merge 1 commit into from
+1 −1

Conversation

thesamesam
Copy link

@thesamesam thesamesam commented Oct 21, 2024 •
edited by rousskov
Loading 

AR="x86_64-pc-linux-gnu-ar"
checking for ar... /bin/false
checking for x86_64-pc-linux-gnu-ar... /bin/false
...
libtool: link: /bin/false cr .libs/libcompatsquid.a...
make[1]: *** [Makefile:899: libcompatsquid.la] Error 1

Use AC_CHECK_TOOL that checks the environment variable $AR as well,
which is useful when cross-compiling (e.g., on Gentoo).

Autoconf v2.72 introduces AC_PROG_AR which does the same thing, but we
do not want to require that 2023 Autoconf version at this time.

TODO: We should be using Automake's AM_PROG_AR (available since 2011
Automake v1.11.0a) instead of manually creating AR_R. AM_PROG_AR also
uses AC_CHECK_TOOLS internally but checks for more ar tool variations.

Gentoo bug report: https://bugs.gentoo.org/911945

@thesamesam
configure.ac: use AC_CHECK_TOOL for ar
e9d43d7 
Use AC_CHECK_TOOL which checks the environment variable `$AR` as well, which
is useful for us in Gentoo when cross-compiling.

We could use AC_PROG_AR in newer autoconf or AM_PROG_AR in automake but
the AR_R use is hardcoded in a bunch of places so not worth it.

Bug: https://bugs.gentoo.org/911945
@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 21, 2024
@rousskov rousskov changed the title configure.ac: use AC_CHECK_TOOL for ar Fix cross-compilation on Gentoo Oct 21, 2024
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 21, 2024
@rousskov
Copy link
Contributor

rousskov commented Oct 21, 2024 •
edited
Loading

@thesamesam, thank you for fixing this bug and sharing your changes!

I fixed PR description formatting (to comply with Squid Project requirements) and adjusted PR title/description text a bit. Please check that I did not misrepresent anything and adjust further as needed. PR title/description will become commit title/message when your changes are automatically merged into master...

Please add one of the two variations of your email addresses to Squid CONTRIBUTORS file. You will find the corresponding diff showing both variations in the failed CI source-maintenance-tests log (expand "Check source-maintenance" section).

I also have a question: Do you know why the following test in Gentoo build.log has failed? Should not ./configure find x86_64-pc-linux-gnu-ar binary if the tester was trying to use that binary by setting $AR to x86_64-pc-linux-gnu-ar?

AR="x86_64-pc-linux-gnu-ar"
...
checking for x86_64-pc-linux-gnu-ar... /bin/false

Disclaimer: I am not a cross-compilation expert.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Oct 21, 2024
@thesamesam
Copy link
Author

Thank you for your feedback! I will address it likely tomorrow if not tonight.

@yadij
Copy link
Contributor

yadij commented Oct 22, 2024

TODO: We should be using Automake's AM_PROG_AR (available since 2011 Automake v1.11.0a) instead of manually creating AR_R. AM_PROG_AR also uses AC_CHECK_TOOLS internally but checks for more ar tool variations.

Would you be interested in doing a followup PR making that change?
If not I can give it a try, but do not have Gentoo access to confirm it will not break things again for you.

yadij
yadij requested changes Oct 22, 2024
View reviewed changes
configure.ac
@@ -129,7 +129,7 @@ AS_IF([test "x$ac_cv_path_PERL" = "xnone"],[
AC_PATH_PROG(POD2MAN, pod2man, $FALSE)
AM_CONDITIONAL(ENABLE_POD2MAN_DOC, test "x${ac_cv_path_POD2MAN}" != "x$FALSE")

AC_PATH_PROG(AR, ar, $FALSE)
AC_CHECK_TOOL(AR, ar, :)
Copy link
Contributor

Choose a reason for hiding this comment

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

The /bin/false alternative is there intentionally to halt early "on first use". Using : will silently skip problems until a lot more build cycles have been wasted.

Suggested change
AC_CHECK_TOOL(AR, ar, :)
AC_CHECK_TOOL(AR, ar, $FALSE)

Copy link
Contributor

Choose a reason for hiding this comment

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

@thesamesam, if you commit Amos suggestion, please do not forget to adjust PR description that currently claims that we are emulating AC_PROG_AR in this fix. AC_PROG_AR does not use $FALSE. It uses : or, effectively, "true" AFAICT:

AC_DEFUN([AC_PROG_AR],
[AC_CHECK_TOOL(AR, ar, :)])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousskov rousskov rousskov left review comments

@yadij yadij yadij requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thesamesam @squid-prbot @rousskov @yadij @squid-anubis