-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
aix,ibmi: Fix compilation errors in fs_copyfile #4404
aix,ibmi: Fix compilation errors in fs_copyfile #4404
Conversation
On IBM AIX (and PASE for IBM i), use st_timespec_t when _XOPEN_SOURCE>=700 and _ALL_SOURCE is defined. Signed-off-by: Jeffrey H. Johnson <trnsz@pobox.com>
Can we use uv_timespec_t here instead? |
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/2287/ The IBM i CI is now passing: https://ci.nodejs.org/job/libuv-test-commit-ibmi/1378/ AIX has failed in not ok 50 - fs_copyfile
# exit code 393222
# Output from process `fs_copyfile`:
# Assertion failed in test/test-fs-copyfile.c on line 145: `r == 0` (-22 == 0) |
In the AIX build log I see the next warning:
Maybe I needs a more specific condition for |
Sorry, I'll take a look again later today. I made sure it compiled on both AIX and i, but I only ran the full tests under i (which passed), which apparently wasn't sufficient. |
All versions of AIX < 7.2 are unsupported and EOL. PASE on i 7.2 (lowest IBM i supported) is based on AIX 7.1, so I believe that checking for just AIX is sufficient, since libuv doesn't target any lower versions. I'm not sure what the policy is for this though. I only have access to i 7.5 to test against. I'll take a look at the test failing later, unless someone beats me to it. |
FYI I'm currently investigating |
I agree with @juanarbol in #4404 (comment) Since the method expects timespec we should just use that instead of st_timespec. Like @johnsonjh noted in #4403 (comment) The AIX sys/stat.h header file contains #if _XOPEN_SOURCE>=700
st_timespec_t st_atim; /* Time of last access */
st_timespec_t st_mtim; /* Time of last data modification */
st_timespec_t st_ctim; /* Time of last file status change */
#else
time_t st_atime; /* Time of last access */
int st_atime_n;
time_t st_mtime; /* Time of last data modification */
int st_mtime_n;
time_t st_ctime; /* Time of last file status change */
int st_ctime_n;
#endif Also found that even if _XOPEN_SOURCE>=700 is set we still have a definition for #if _XOPEN_SOURCE>=700
#define st_atime st_atim.tv_sec
#define st_mtime st_mtim.tv_sec
#define st_ctime st_ctim.tv_sec
#define st_atime_n st_atim.tv_nsec
#define st_mtime_n st_mtim.tv_nsec
#define st_ctime_n st_ctim.tv_nsec
#define st_pad1 st_atim.tv_pad
#define st_pad2 st_mtim.tv_pad
#define st_pad3 st_ctim.tv_pad
#endif There is also a difference between timespec.tv_nsec and st_timespec.tv_nsec if _XOPEN_SOURCE>=700 is defined #if _XOPEN_SOURCE>=700
#ifndef _TIMESPEC
#define _TIMESPEC
struct timespec {
time_t tv_sec; /* seconds */
long tv_nsec; /* and nanoseconds */
};
#endif
/* internal timespec to preserve binary compatibility */
typedef struct st_timespec {
time_t tv_sec; /* seconds */
int tv_nsec; /* and nanoseconds */
} st_timespec_t;
#endif So what I did was just use From 34777ede7bffcc817043e683018c4aac29bfcee5 Mon Sep 17 00:00:00 2001
From: Abdirahim Musse <33973272+abmusse@users.noreply.github.com>
Date: Wed, 15 May 2024 16:19:40 -0500
Subject: [PATCH] fix(aix/ibmi): timespec compliation error
---
src/unix/fs.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/unix/fs.c b/src/unix/fs.c
index 0c6c585c6d6..c4eadd63d8e 100644
--- a/src/unix/fs.c
+++ b/src/unix/fs.c
@@ -1317,6 +1317,11 @@ static ssize_t uv__fs_copyfile(uv_fs_t* req) {
#if defined(__APPLE__)
times[0] = src_statsbuf.st_atimespec;
times[1] = src_statsbuf.st_mtimespec;
+#elif defined(_AIX)
+ times[0].tv_sec = src_statsbuf.st_atime;
+ times[0].tv_nsec = src_statsbuf.st_atime_n;
+ times[1].tv_sec = src_statsbuf.st_mtime;
+ times[1].tv_nsec = src_statsbuf.st_mtime_n;
#else
times[0] = src_statsbuf.st_atim;
times[1] = src_statsbuf.st_mtim; For the AIX test build it looks like For the IBM i test build does define BTW the test builds were from my fork and branch with this commit: abmusse@34777ed From the the above test builds looks like |
@abmusse thanks for investigating <3; I don't have access to a IBMi nor a AIX system and I'm the one who broke the whole thing. I'm sorry for not being that helpful on this one. |
Thanks for chiming in! I can understand why it wasn't obvious that things broke. We need to get CI AIX machine back to green so that it becomes more obvious that a PR caused a failure. ref: #4231 I will look into that failure some more now that I have some free cycles to investigate. |
I haven't tested the revised patch from @abmusse yet... but I should be able to in the next day or so on i 7.5 and latest TL/SP AIX 7.2 and 7.3 in the next day or so. |
@abmusse FYI, (So any solution only needs to work with it, since there isn't any possible build that doesn't define it – and not defining it would likely introduce other breakage that would need to be fixed – so I think the solution is OK. The |
b381ff5
to
b7ebd37
Compare
…jh/libuv into johnsonjh/20240512/st_timespec_t
Updated with the solution from #4404 (comment) for further testing. |
…jh/libuv into johnsonjh/20240512/st_timespec_t
I tried to trigger the jenkins job for this, but apparently I don't have permissions. @richardlau is it possible that I could get permissions? |
Done |
I think AIX is now happy. See https://ci.nodejs.org/job/libuv-test-commit-aix/nodes=aix72-ppc64/2305/console |
Looks OK in my testing. |
I think things are looking good in this PR. Do you know who should be added as a reviewer so that we can get this reviewed and merged? |
Fix compilation on IBM AIX and OS/400 (PASE for IBM i).
Closes #4403