I prefer to do the review via email rather than in the Savannah bug tracker which has pretty annoying markup.
I would appreciate a somewhat comprehensive commit message or ChangeLog for this set of patches, at least explaining some of the less obvious modifications. > +set -x > +if [ ! ${PLATFORM} = "OS/390" ]; then > +$CC $CFLAGS $LDFLAGS -L"$OUTLIB" -o "$OUTDIR/makenew$EXEEXT" $objs - > lgnu $LOADLIBES > +else > $CC $CFLAGS $LDFLAGS -L"$OUTLIB" $objs -lgnu $LOADLIBES -o > "$OUTDIR/makenew$EXEEXT" > +fi We don't want set -x here. Is the point of this that the compiler on OS/390 doesn't allow the -o option to come after the objects? If so we should just change the command line order on all systems; no need to check for platforms here. Other compilers don't care about the order in which -o comes so it can just come early for all of them. > -# define __stat stat > +# define __gnustat stat I suppose OS/390 already defines __stat to something else? All this code in glob.c and fnmatch.c is not really owned by GNU make, we import it from elsewhere. But it looks like we'll have to do something about this. > +#ifdef __MVS__ > +int execvpe(const char* name, > + char* const argv[], > + char* const envp[]) { Please follow the GNU coding style here and in all code. That means the return type on its own line, braces on their own lines (both function and inner block braces), one space between a function name and the open paren, etc. You can find info here: https://www.gnu.org/prep/standards/html_node/Formatting.html Also, I'm not sure why we need this function. Can you provide some comments or similar here explaining why it's needed and what it does? > +int __setccsid(int fd, int ccsid) > +{ > + attrib_t attr; > + int rc; > + As above please follow the coding standards. Also some comments here are needed to explain what this is doing. I also wonder if it wouldn't be better to collect all this z/OS support into a separate C file rather than sprinkling it throughout the rest of the code (I realize that's the model that was used in the past but it's not a good one). Finally, I don't think it's a good idea to use identifiers that start with two underscores like this (__setccsid) because these are reserved for use by the compiler, according to the C standard. Instead we should choose a name that won't conflict, unless there's some reason that these functions must have this specific name (like you're overriding a system function or something). > +#ifdef __MVS__ > +#include "os390.h" > +# include <fcntl.h> > +# define pipe(_p) __pipe_ascii(_p) > +#endif It seems like it shouldn't be needed to include "os390.h" here. Especially if the custom parts of z/OS are restricted to a new file, this header can just be included there I expect. > +#ifndef __MVS__ > + //TODO: Implement alternative > void *sem = acquire_semaphore (); > +#endif I don't think this is the right way to handle this. If the port doesn't support output sync, then you should ensure that NO_OUTPUT_SYNC is defined for this port, then none of this code will be compiled. > /* These track the state of the jobserver pipe. Passed to child > instances. */ > -static int job_fds[2] = { -1, -1 }; > +static int job_fds[2] = {}; I'm pretty sure this will break other ports. Why is it needed? > + if ($osname eq "") { > + eval "chop (\$osname = `uname -nmsr 2>&1`)"; By ignoring the comment here about /bin/sh -c it seems you'll cause issues with other ports. Can you explain why this is needed? If /bin/sh -c doesn't work on zOS maybe we need to find a better way to handle this. Also I don't understand why you're retesting $osname directly after it was just tested. In general I'm always happy to receive ports to new platforms but I will remind that I can't test these going forward. Hopefully someone on the IBM team will commit to subscribing to the bug-make@gnu.org list and signing up to test pre-releases when they come out, to make sure that the zOS support is still working. Thanks for submitting this, and I hope the review was useful!