search

bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
4.15k stars 234 forks source link

Get the POSIX strerror_r(3) definition with glibc #836

Closed tleedjarv closed 1 year ago

tleedjarv commented 1 year ago

glibc includes a non-POSIX version of strerror_r(3). POSIX-compliant version of strerror_r(3) is the default but some feature test macros must be defined in case CFLAGS change the default settings.

The same result could be achieved by just defining _DEFAULT_SOURCE but this is available only since glibc 2.19.

gdt commented 1 year ago

We need to tread carefully here. I have seen a lot of cases where feature macros are defined, but the code uses things not defined by that feature. Generally, systems hide everything not required by a defined macro.

I don't follow where CFLAGS that ask for something else are coming from. That seems like an error compared to what BUILDING.md should say. I wonder about detecting bad flags and #error, but I'll try to build and understand this better.

tleedjarv commented 1 year ago

This was just a precaution. We may just leave it be and deal with it when any actual issues surface. The more I think about it, the more this seems like the right way to go.

We need to tread carefully here. I have seen a lot of cases where feature macros are defined, but the code uses things not defined by that feature. Generally, systems hide everything not required by a defined macro.

My thinking is this. We don't target glibc, hence it is not only safe but also desirable to undefine _GNU_SOURCE. We do target POSIX, including XSI and anything non-POSIX is always guarded. Defining _POSIX_C_SOURCE should not only be safe but also desirable. Or it could be (sometimes must be) replaced by a definition of _XOPEN_SOURCE. Perhaps it is still useful to define _DEFAULT_SOURCE, too.

As to hiding everything not required, this is expected. This specific file is using non-standard interfaces and is very heavily guarded already. strerror_r is almost the only standard thing in there :D It uses very few library functions overall.

I don't follow where CFLAGS that ask for something else are coming from. That seems like an error compared to what BUILDING.md should say. I wonder about detecting bad flags and #error, but I'll try to build and understand this better.

Those CFLAGS come from packaging systems. In two ways. Many (most? all?) packaging systems provide their own CFLAGS when building projects. Unison's Makefile currently uses CFLAGS incorrectly but I have a patch fixing that (PR coming soon).

Second, the OCaml compiler itself will record CFLAGS used to compile it and will use those flags when itself compiling other files. You can see this with ocamlc -config or if you add -verbose to CAMLFLAGS in Makefile.OCaml.

tleedjarv commented 1 year ago

I'll close this PR for now. It can be reopened whenever there's an actual issue.