gh-142379: configure.ac: detect unusable termio operations#142380
gh-142379: configure.ac: detect unusable termio operations#142380tpetazzoni wants to merge 1 commit intopython:mainfrom
Conversation
tpetazzoni
requested review from
AA-Turner,
corona10,
emmatyping and
erlend-aasland
as code owners
tpetazzoni
force-pushed
the
check-tc-operations
branch
2 times, most recently
from
fd47b13 to
d766640
Compare
| dnl PY_CHECK_IOCTL(IOCTL_SYMBOL) | ||
| AC_DEFUN([PY_CHECK_IOCTL], | ||
| [ | ||
| AC_MSG_CHECKING([for $1]) | ||
| AC_COMPILE_IFELSE( | ||
| [AC_LANG_PROGRAM( | ||
| [[#include <sys/ioctl.h>]], | ||
| [[ | ||
| /* Test whether $1 is declared */ | ||
| long val = $1; | ||
| return 0; | ||
| ]] | ||
| )], | ||
| [AC_MSG_RESULT([yes]) | ||
| AC_DEFINE([HAVE_$1], [1], | ||
| [Define this if $1 termio operation is usable])], | ||
| [AC_MSG_RESULT([no])] | ||
| ) | ||
| ]) |
There was a problem hiding this comment.
Can you make this a generic function instead? it's useful for checking the presence of a macro. Make the includes and the expected macro type parameters of that function, as well as the name of your HAVE_ macro as you did. This could be useful elsewhere.
Ina ddition, there is no need for the return 0; as it's implicit. Also remove `/* Test whether $1 is declared */ because it's trivial what it does.
There was a problem hiding this comment.
Thanks for the feedback @picnixz. Actually such a generic macro already "exists" to some degree, it's AC_CHECK_DECL. However, we can't use it in our specific case because what AC_CHECK_DECL does is:
#ifndef YOUR_DECLARATION
(void) YOUR_DECLARATION;
#endif
The idea being: the #ifndef test if it is a macro => if it is, then fine, we're good. If it's not a macro, it checks if it's a valid r-value.
In our case, TCGETA is a macro, BUT we need to check that it is actually usable, not just "defined". And that's why AC_CHECK_DECL doesn't do the trick.
If you still think a generic macro would be good, what name should it have? Indeed, PY_CHECK_DECL would be very confusing if it has this subtle difference with AC_CHECK_DECL. Or maybe PY_CHECK_USABLE_DECL ?
And of course, our macro isn't as generic as AC_CHECK_DECL, because if the declaration exists but has no value (like #define FOO), then our test will fail.
There was a problem hiding this comment.
My idea was actually different. What I wanted to test is:
TYPE holder = $1;
where you specify TYPE (in your case it's long). And for the name, it would be PY_CHECK_CONSTANT. I wanted you to actually remove the comment but not the test itself.
There was a problem hiding this comment.
Makes a lot of sense, will do!
tpetazzoni
force-pushed
the
check-tc-operations
branch
from
d766640 to
7ba29d0
Compare
|
@picnixz I just pushed a new version of this branch, which normally takes into account your proposed changes. Let me know what you think. |
tpetazzoni
force-pushed
the
check-tc-operations
branch
2 times, most recently
from
cdd35f0 to
5e7390b
Compare
|
The CI fails with "Error: The template is not valid. .github/workflows/build.yml (Line: 715, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0." which doesn't make much sense to me. |
This was due to a change in the pipeline definitions I believe. I've merged in main which should fix this issue. |
tpetazzoni
force-pushed
the
check-tc-operations
branch
from
f9abc5f to
21e3150
Compare
tpetazzoni
force-pushed
the
check-tc-operations
branch
from
21e3150 to
9b68b03
Compare
Some termio operations are not actually usable on some architectures. For example, the TCGETA, TCSETA, TCSETAF and TCSETAW are defined with a reference to "struct termio" on alpha, hppa and sparc64, but "struct termio" is no longer defined since glibc 2.42, causing a build failure. Instead of using those operations as soon as they are defined, this commit checks more carefully that they are actually usable. Such a check cannot be done with the standard AC_CHECK_DECL() m4 macro, as this macro only checks that the #define exists, not that it is actually usable (i.e builds properly). Therefore, we introduce a PY_CHECK_CONSTANT() m4 macro, that verifies that the constant is actually usable. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
tpetazzoni
force-pushed
the
check-tc-operations
branch
from
9b68b03 to
7b99166
Compare
| @@ -0,0 +1,9 @@ | |||
| Some termio operations are not actually usable on some | |||
There was a problem hiding this comment.
Why is the NEWS entry still long? I think you overwrote your change.
| AC_DEFINE([HAVE_$2], [1], | ||
| [Defined if $2 is usable])], |
There was a problem hiding this comment.
| AC_DEFINE([HAVE_$2], [1], | |
| [Defined if $2 is usable])], | |
| AC_DEFINE([HAVE_$2], [1], [Defined if $2 is usable])], |
(Sorry, but I really want to avoid vertically expanding our configure.ac as much as possible; ideally we should have all "functions" in a separate file but we don't do that...)
|
Ping @tpetazzoni: Can you address @picnixz's review? |
|
I just come across the same issue in another piece of software. To me, it look like a distribution bug (the structures are used in kernel headers provided by the distribution, but come from glibc, - a distribution didn't update the two in one go). I'd say everyone should not jump through hoops on this but ask distribution(s) to fix the mess. This is definitely a build environment problem, not the software-being-built problem. FWIW. |
|
The fix hopefully should be in linux soon. E.g. https://lore.kernel.org/sparclinux/123e6a3a-6360-45cb-8eef-23b1660b9253@gaisler.com/T/#t |
@mjt0k Were the patches accepted? |
|
-1; I don't think we should add workarounds for new bugs unsupported architectures. But if y'all disagree: can we start using |
|
This PR is stale because it has been open for 30 days with no activity. |
7 participants
Some termio operations are not actually usable on some architectures. For example, the TCGETA, TCSETA, TCSETAF and TCSETAW are defined with a reference to "struct termio" on alpha, hppa and sparc64, but "struct termio" is no longer defined since glibc 2.42, causing a build failure.
Instead of using those operations as soon as they are defined, this commit checks more carefully that they are actually usable. This is done using a new m4 macro PY_CHECK_IOCTL.