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

Correct cpu features detection during cross-compiation #4151

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

aol-nnov
Copy link
Contributor

Closes #4150

@CLAassistant
Copy link

CLAassistant commented Nov 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@sauwming sauwming left a comment

Choose a reason for hiding this comment

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

You also need to commit the modified aconfigure as well (i.e. autoconf aconfigure.ac > aconfigure).

aconfigure.ac Outdated Show resolved Hide resolved
aconfigure.ac Outdated Show resolved Hide resolved
@aol-nnov
Copy link
Contributor Author

You also need to commit the modified aconfigure

Should I include aclocal.m4 as well? AX_CHECK_COMPILE_FLAG is an external macro, provided by autoconf-archive package in Debian.

@sauwming
Copy link
Member

You also need to commit the modified aconfigure
Should I include aclocal.m4 as well? AX_CHECK_COMPILE_FLAG is an external macro, provided by autoconf-archive package in Debian.

Then this could be a problem. I check my Mac machine and it doesn't have autoconf-archive installed by default. Try to replace it with readily available macros instead, such as AC_COMPILE... or something else.

@aol-nnov
Copy link
Contributor Author

Then this could be a problem

But why? If I run aclocal, all external macro are copied over to self-sufficient aclocal.m4 and I can include it into the commit as well.

@aol-nnov
Copy link
Contributor Author

Also, autoconf-archive is available on Mac via HomeBrew: https://formulae.brew.sh/formula/autoconf-archive#default

@aol-nnov aol-nnov force-pushed the cpu-features-detection branch 2 times, most recently from 33e8a26 to 4d4db3a Compare November 12, 2024 11:15
@aol-nnov
Copy link
Contributor Author

@sauwming Let's do another round. I've implemented your comments.

Also, please check aclocal.m4 if it works for you that way.

@sauwming
Copy link
Member

sauwming commented Nov 12, 2024

The aclocal.m4 seems to work okay for me, as well as on the CI machines.
Although quite frankly, it still leaves me worried.

====
aclocal.m4 itself is not inherently portable across different environments without considering some nuances.

Portability Considerations:

  • Generated by aclocal:
    aclocal.m4 is typically generated on the build machine where aclocal runs. The macros it includes are influenced by the m4 macro files available on that machine.
  • Environment-Specific Dependencies:
    If aclocal.m4 depends on custom or third-party macros (e.g., from the Autoconf Archive), it may reference macros that are not present on another system. This can lead to portability issues when transferring the aclocal.m4 file to a different environment without the necessary macro definitions.
  • Not a Replacement for Autoconf and Automake:
    While you can transfer aclocal.m4 to another system, it’s not a full replacement for running aclocal, automake, or autoconf. The environment where the configure script is regenerated must have the necessary tools and macro files.

Conclusion:
aclocal.m4 is not highly portable by itself because it relies on macros defined by the environment in which it was generated. For distributing software, ensure that you distribute the configure script and any other build system files needed for an end-user to compile the code without requiring Autotools.

====

I know I can install autoconf-archive, but I don't want to do this on every machine and I don't wish to make this a requirement, especially if it's not really necessary.

So, I would prefer if we check the availability of compile flags using the standard macros instead, like everywhere else in the currently existing configure script.

@aol-nnov
Copy link
Contributor Author

aol-nnov commented Nov 12, 2024

So, you want me to copy and paste aclocal.m4 contents inside the aconfigure.ac? Okay, will do that tomorrow.

I'm out for today and will be back online in next 12 hours or so.

Thank you for the cooperation!

@sauwming
Copy link
Member

sauwming commented Nov 12, 2024

I believe you can replace it with AC_COMPILE_IFELSE.

Don't worry, no hurry.

@aol-nnov
Copy link
Contributor Author

@sauwming I've rewrote the patch with AC_COMPILE_IFELSE as you proposed earlier. Please review.

As for factoring out aarch64*) into separate branch, I had a small chat with our HW team and they had some uncertanities about the fact, that all aarch64 CPUS have neon support. On the other hand, I have no access to arm-apple-darwin* hw to check.

So, it's the least intrusive patch for now.

@sauwming sauwming merged commit 4a8d180 into pjsip:master Nov 14, 2024
36 checks passed
@aol-nnov aol-nnov deleted the cpu-features-detection branch November 14, 2024 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct CPU (compiler) features detection during cross-compile
4 participants