[bug #62654] Add z/OS support

[Paul D. Smith]

Update of bug #62654 (project make):

  Status:None => Fixed  
 Assigned to:None => psmith 
 Open/Closed:Open => Closed 
   Fixed Release:None => SCM
   Triage Status:None => Medium Effort  

___

Follow-up Comment #9:

I pushed changes to support z/OS.  Thanks for use of the test system!


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[igor.todorovski]

Follow-up Comment #8, bug #62654 (project make):

My apologies, I uploaded the wrong one. Here's the z/OS config.log

(file #54183)

___

Additional Item Attachment:

File name: config.log Size:318 KB




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[Paul D. Smith]

Follow-up Comment #7, bug #62654 (project make):

Also it appears to be from a build on a GNU/Linux x64 system, not z/OS???


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[Paul D. Smith]

Follow-up Comment #6, bug #62654 (project make):

I did get your emails, sorry for not replying Igor.

I took a look at the config.log you attached and it appears to be from a
different package, not from GNU make, unfortunately.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[igor.todorovski]

Follow-up Comment #5, bug #62654 (project make):

Hi Paul,

I think my emails are likely not getting to you and the mailing list button is
not working with outlook for some reason. Is there a way to get the "email
address" to reply to a thread?

Anyyway, this is my Make 4.4 z/OS patch:
https://github.com/ZOSOpenTools/makeport/blob/main/patches/PR1.patch

It’s a lot simpler compared to the 4.3 changes since I’m now linking to
our zoslib library which consolidates a lot of the common changes we’re
making to OSS packages.

I also uploaded the config.log.

(file #54063)

___

Additional Item Attachment:

File name: config.log Size:417 KB




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

RE: [bug #62654] Add z/OS support

[rsbecker]

On July 3, 2022 7:33 PM, Paul Smith wrote:
>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.

I encountered the issue that the z/OS xlc compiler needs -o file ahead of all 
other objects on the command line. Definitely non-standard.

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

stat also actually needs stat64 to get past the UNIX 2038 rollover.

Regards,
Randall

RE: [bug #62654] Add z/OS support

[Paul Smith]

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 
> +# 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!

[bug #62654] Add z/OS support

[igor.todorovski]

Follow-up Comment #3, bug #62654 (project make):

Hi Paul, I'm just checking in.  Do you have an update on the review of the
z/OS enablement patch?  

If you have any comments about the patch, please let me know.

Thanks,
Igor




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[igor.todorovski]

Follow-up Comment #2, bug #62654 (project make):

Sure, here's the patch.

Some notes:
* Adds a os390.h file to implement some os390/z/OS specific functions
* All changes should be guarded by the __MVS__ macro 
* Assumes xlclang as the C/C++ compiler

(file #53326)

___

Additional Item Attachment:

File name: initial_port.patch Size:17 KB




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[Paul D. Smith]

Follow-up Comment #1, bug #62654 (project make):

You can attach the patch here and we will take a look.  You don't have to make
it work with current Git master HEAD.



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #62654] Add z/OS support

[igor.todorovski]

URL:
  

 Summary: Add z/OS support
 Project: make
   Submitter: igortodorovski
   Submitted: Mon 20 Jun 2022 03:22:05 PM UTC
Severity: 3 - Normal
  Item Group: Enhancement
  Status: None
 Privacy: Public
 Assigned to: None
 Open/Closed: Open
 Discussion Lock: Any
   Component Version: 4.3
Operating System: POSIX-Based
   Fixed Release: None
   Triage Status: None


___

Follow-up Comments:


---
Date: Mon 20 Jun 2022 03:22:05 PM UTC By: igor.todorovski 
Hi, I have a patch to add z/OS ASCII support, which applies to Make v4.3 only.
 Do I need to make my patch apply to master or can I submit it for 4.3?







___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/