More configure fixes #13164
More configure fixes #13164
Conversation
The manual states it was introduced in Mac OS X 10.12 (2016). I have no proof, but it's likely that it was never made available in another header. |
I had misread the changes to |
|
I second @dra27 in finding every aspect of this work excellent.
Thanks a lot, @MisterDA.
Re:the commit entitled "Keep former macros for compatibility", two
questions:
1. Wouldn't it be better to split it and to squash each of its parts in
the commit that removes the corresponding macros?
2. Would it make sense to ensure that the compatibility macros are
defined only outside of the runtime, to make sure we do not ourselves
restart using them?
You mayalso want to add a comment sayiing at which version the macro got
deprecated, so that it's easier to decide, later, that time for removal
has arrived.
In reference to what you wrote in the commit message: "If this list
keeps growing, it could be moved to a separate file.": it might be more
useful to have this comment in the code itself rather than in the commit
message. Also, after all, why not introducing the file right now and so
it would be its inclusiob which could be protected by ifdef not
CAML_INTERNAL.
And, out of curiosity, re:"add missing quotes in branch", you mention
that using autoconf-2.70 gives an error but I thought we were usignit
already so how is it possible that this got unnoticed?
|
|
David Allsopp (2024/05/15 04:19 -0700):
I like every aspect of this PR, thank you 🙂 Adding a green tick, but
I'll let @shindere have a chance to commit before merging.
Many thanks for having reviewed AND given me the opportunity to do
likewise! Would have been a pity to miss such a nice work.
|
Will do.
Great idea, I'll re-introduce
Without the |
|
Antonin Décimo (2024/05/15 22:10 -0700):
> 1. Wouldn't it be better to split it and to squash each of its parts in the commit that removes the corresponding macros?
Will do.
Cool, thanks!
> 2. […] Also, after all, why not introducing the file right now and so it would be its inclusion which could be protected by `#ifndef CAML_INTERNAL`.
Great idea, I'll re-introduce `compatibility.h`.
Thanks! That, or `legacy.h` as matches your taste best.
> And, out of curiosity, re:"add missing quotes in branch", you mention that using autoconf-2.70 gives an error but I thought we were using it already so how is it possible that this got unnoticed?
Without the `[ … ]` quotes, the code is brittle and sensitive to the
context. When I removed other occurrences of `AC_CHECK_DECL` which
occurred before this bug, Autoconf stopped expanding the macro before
the bug, but expanded it at its sole occurrence inside the expansion
of the `AS_IF` macro, which led to a wrong quoting, wrong nesting of
if blocks and parentheses.
Ah okay so it's because it was fragile, not because it was strictly
wrong, AIUI. Thanks for the explanation, it's enlightening!
|
MisterDA
force-pushed
the
configure-fixes
branch
2 times, most recently
from
1bfba09
to
123a2f2
Compare
|
Rebased. I went with |
MisterDA
force-pushed
the
configure-fixes
branch
2 times, most recently
from
2bbc52b
to
bb0b6f7
Compare
|
Rebased for the MSVC CI fix. |
|
David Allsopp (2024/05/16 00:22 -0700):
We can't co-opt `CAML_INTERNALS` this way, I'm afraid. Defining `CAML_INTERNALS` means you've opted _in_ to the internals; its negation does not mean that we're building OCaml itself!
Ah, makes sense indeed.
Apologies for the misleading suggestion.
FWIW I wouldn't worry about guarding this against internal usage, so
I'd just unconditionally define them, as before.
OK, I guess we could always have a CI test to ensure that none of the
macros defined in the compatibility header is actually used by the rest
of the system, if we wanted.
|
|
Sorry, I'm getting confused: didn't we agree that "Keep former
Autoconf-defined macros for compatibility" would be split and melt into
the previous commits?
If I may nitpick about the name of the compatibility header: do we
really care that the macros come from `config`? Can't there be macros
from other places that we maywant to deprecate and whose definition
would move to a compatibility header during the deprecation period?
|
I split the commit in two: we already had a definition of
We have the example of the |
Fixes error with stricter quoting requirements of autoconf-2.70.
Support for getentropy on macOS was already correctly detected in the C code, but it's nicer to detect it in the configure script, otherwise Autoconf prints a message telling it couldn't detect getentropy. The includes are not needed to check if getentropy is present.
The macro was never used.
It can be moved to the preprocessor completely.
The following variables stopped to occur in other files than configure.ac, thus ceased to need to be substituted in other files. flexlink_flags, mkexe_extra_flags, mkdll_ldflags_exp, mkexe_ldflags_exp: 319b77f target_bindir: 8681e66 The following never occurred in other files so don't need to be substituted. has_monotonic_clock: b7f0494 The following is probably a left-over or a duplicate of EXEC_MAGIC_LENGTH and has never been used. MAGIC_NUMBER_LENGTH: 7892e6f
|
Antonin Décimo (2024/05/17 07:06 -0700):
OK works for me for the commits and yes please lets rename the header to
the more general! :)
|
Done. |
Inline otherlibs/unix/nanosecond_stat.h into otherlibs/unix/stat_unix.c since the latter is the only user. Use the Autoconf-generated macros for each member name rather than an ad-hoc macro. Keep the former macro HAS_NANOSECOND_STAT for compatibility.
No need to get the headers right, the program will link anyway.
The Autoconf manual suggests not to rely on the default value (1).
The diff was generated with:
sed -i -E 's/AC_DEFINE\(\[([_A-Z0-9]*)\]\)/AC_DEFINE([\1], [1])/g' \
configure.ac aclocal.m4
This PR attempts to remove accumulated Autoconf cruft, outdated code/comments, and fix bugs.
Unfortunately the commit
configure: set the value of AC_DEFINE macros to 1adds some noise to this PR.No change entry needed.
cc @shindere