zstd
zstd copied to clipboard
facebook
Fix build failure in Alpha architecture
Taken from Debian package, see https://bugs.debian.org/962676 If it would be merged, we Debian can reduce distro-specific patches. Please consider applying, thanks.
Hi @henrich!
Thank you for your pull request and welcome to our community.
Action Required
In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.
Process
In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.
Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.
If you have received this in error or have any questions, please contact us at [email protected]. Thanks!
I'm uncomfortable with this fix as proposed. While this is a reasonable patch for Debian to ship, since they can make guarantees about the environment this code is being shipped in, I don't think we can do the same. So I agree with @Cyan4973--I can only assume there are old or maybe even current OS runtimes for Alpha that define __alpha__ but which do not have st_mtime.
We need a good signal to detect this capability. It sounds like this might be difficult, since it's apparent the environment is non-trivially non-compliant with POSIX. (For what it's worth, the linked bug is incorrect: POSIX 2008 requires that st_mtime is a macro: see page 370 line 12824 of this pdf. And POSIX 2001 requires struct utimbuf: page 1563 line 48237 of this pdf.)
In the absence of anything better, we could just disable the utime() path for Alpha, which will cause UTIL_utime() to just fall back to being a no-op.
FYI, this is actually a bug in glibc itself. I'm working on a fix upstream and will report back.
FYI, this is actually a bug in glibc itself. I'm working on a fix upstream and will report back.
Sorry, I was wrong - there is a glibc bug on microblaze, but on alpha the issue is really in zstd itself. Sorry for the noise.
@henrich, @arnout, we're ramping up to our next release and I'd like to get a fix for this issue merged.
I can make the test as follows:
#if ((PLATFORM_POSIX_VERSION >= 200809L) && (defined(st_mtime) || defined(__alpha__)))
struct timespec timebuf;
// ...
#elif (PLATFORM_POSIX_VERSION >= 200112L)
struct utimbuf timebuf;
// ...
#else
// do nothing
#endif
Is it correct to assume all Alpha systems with _POSIX_VERSION >= 200809L expose a struct timespec? Otherwise this exemption is dangerous.
In the absence of any activity, I'm going to close this.
If anyone with an alpha system can comment on the appropriateness of the proposed fix, I'm happy to re-open this and get a fix merged.