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 fs_copyfile:
https://ci.nodejs.org/job/libuv-test-commit-aix/2290/nodes=aix72-ppc64/console

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:

src/unix/fs.c: In function 'uv__fs_copyfile':
src/unix/fs.c:1329:23: warning: passing argument 2 of 'futimens' from incompatible pointer type [-Wincompatible-pointer-types]
 1329 |   if (futimens(dstfd, times) == -1) {
      |                       ^~~~~
      |                       |
      |                       st_timespec_t * {aka struct st_timespec *}
In file included from ./include/uv/unix.h:26,
                 from ./include/uv.h:71,
                 from src/unix/fs.c:29:
/usr/include/sys/stat.h:445:27: note: expected 'const struct timespec *' but argument is of type 'st_timespec_t *' {aka 'struct st_timespec *'}
  445 |  extern int futimens(int, const struct timespec *);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~

Maybe I needs a more specific condition for AIX? and use const struct timespec *

@richardlau

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.

I saw this conditionals which seems to be related to this compile issue:

And this one seems to be validating a specific version of AIX

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.

I'll take a look at the test failing later, unless someone beats me to it.

@johnsonjh

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 st_atime and st_mtime that will point to the right thing. We can just use that and not worry about if _XOPEN_SOURCE>=700 was set or not.

#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 st_atime, st_atime_n, st_mtime, st_mtime_n and copy the values from the statsbuf into the timespec struct fields to build it up.

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 _XOPEN_SOURCE is not passed as -D option but I'm not sure if its set internally by something else.

For the IBM i test build does define -D_ALL_SOURCE -D_XOPEN_SOURCE=500

BTW the test builds were from my fork and branch with this commit: abmusse@34777ed

From the the above test builds looks like fs_copyfile test case passes on both AIX and IBM i.

@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.

@juanarbol

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, _ALL_SOURCE is always defined for libuv builds on both AIX and i.

(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. _ALL_SOURCE has very far reaching effects in many AIX and PASE headers.)

The _XOPEN_SOURCE definition happens in standards.h. On AIX 7.2 and 7.3, when defining _ALL_SOURCE, it causes _XOPEN_SOURCE to be undefined and redefined to 700 explicitly.

Updated with the solution from #4404 (comment) for further testing.

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?

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.