Ticket #3927 (closed enhancement: fixed)

Opened 7 weeks ago

Last modified 2 weeks ago

mc on IBM i PASE building 4.8.20 change

Reported by: jax Owned by: andrew_b
Priority: minor Milestone: 4.8.22
Component: mc-vfs Version: 4.8.20
Keywords: PASE, AIX, iSeries, AS/400 Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

To build 4.8.20 on IBM i 7.3 PASE environment required the following change:

On lines 103-104 of lib/vfs/vfs.h:

// typedef struct timespec mc_timesbuf_t[2];
typedef struct st_timespec mc_timesbuf_t[2];

I am a longtime (20+ years) user of MC on Linux, OpenBSD, Mac and Windows and am thrilled to finally have succeeded in building it on IBM i PASE environment!

Attachments

mc.spec (4.6 KB) - added by jax 4 weeks ago.
RPM spec file IBM i PASE environment build of MC
mc.2.spec (4.6 KB) - added by jax 4 weeks ago.
RPM spec file IBM i PASE environment build of MC
mc_common.c.patch (1.2 KB) - added by jax 4 weeks ago.
Patch regarding openpty()
mc_configure.ac.patch (556 bytes) - added by jax 4 weeks ago.
Patch to configure.ac
patches, 3sep2018.zip (2.9 KB) - added by IBMJesseG 3 weeks ago.
Updated patches and spec file, Sept 2, 2018

Change History

comment:1 Changed 4 weeks ago by andrew_b

Is it enough to check _AIX macro as following:

@@ -100,7 +100,11 @@ typedef void (*fill_names_f) (const char *);
 typedef void *vfsid;
 
 #ifdef HAVE_UTIMENSAT
+#ifdef _AIX
+typedef struct st_timespec mc_timesbuf_t[2];
+#else
 typedef struct timespec mc_timesbuf_t[2];
+#endif
 #else
 typedef struct utimbuf mc_timesbuf_t;
 #endif

Is AIX version check is required?

comment:2 follow-up: ↓ 3 Changed 4 weeks ago by jax

Thank you, andrew_b.

In the time since I opened this ticket, IBM's open source development team has released their own compilation of Midnight Commander.

I am going to download their Source RPMS and do a diff upon that and submit better and more comprehensive changes, including their openpty() changes.

Shall I post back here, or open a new ticket?

comment:3 in reply to: ↑ 2 Changed 4 weeks ago by andrew_b

Replying to jax:

Shall I post back here, or open a new ticket?

Let's here.

comment:4 Changed 4 weeks ago by jax

Thanks, I'll work on that and get back to you.

Changed 4 weeks ago by jax

RPM spec file IBM i PASE environment build of MC

Changed 4 weeks ago by jax

RPM spec file IBM i PASE environment build of MC

comment:5 Changed 4 weeks ago by jax

I'm attaching the patches and RPM spec that were added by IBM i Open Source team to make the system compile on PASE. Oops, I see I attached the .spec file twice :(

Changed 4 weeks ago by jax

Patch regarding openpty()

Changed 4 weeks ago by jax

Patch to configure.ac

comment:6 Changed 4 weeks ago by IBMJesseG

@jax, thanks for submitting this up!

@andrew_b, et al, I'm with the IBM teams and can help answer questions related to the patches @jax has submitted. My apologies for the sloppiness in the common.c patch (checking #ifdef HAVE_OPENPTY but commenting #endif PASE, etc).

comment:7 Changed 3 weeks ago by IBMJesseG

The utimensat hack in the configure.ac could be wrapped with an "os400" host check (as you can see in the spec file, we build with "powerpc64-ibm-os400") until we find a more-correct long term fix.

comment:8 Changed 3 weeks ago by andrew_b

What is the more correct way: check $host_os like

case $host_os in
*os400)
   ;;
*)
   AC_CHECK_FUNCS([utimensat])
   ;;
esac

or use IS_AIX macro defined in configure.ac?

comment:9 Changed 3 weeks ago by andrew_b

In the mc.spec, I can see there are some strange things.

BuildRequires?: boost

MC is writen in pure C, therefore any C++ library is not needed.

BuildRequires?: libxml2-devel
BuildRequires?: libxml2-2
BuildRequires?: libxml2-tools
BuildRequires?: libxslt-devel

MC doesn't process any XML data inside. XML library is not needed.

BuildRequires?: unixODBC-devel
BuildRequires?: curl-devel
BuildRequires?: openssl-devel < 1.1.0

Another unnecessary dependencies.

BuildRequires?: libutil-devel

Can't say what it is.

BuildRequires?: slang-devel

You want buld MC with S-Lang, but run configure with "--with-ncurses-libs=/QOpenSys/pkgs/lib" option.

comment:10 Changed 3 weeks ago by IBMJesseG

Andrew, the "case $host_os in *os400" is the correct way, since the change is not needed for AIX.

The grade for mc.spec: D+
The .spec file is admittedly sloppy as a result of being derivative of another .spec file. I'll clean up those extraneous BuildRequires?. libutil-devel is an IBMi-only package and is needed for the IBM i implementation of openpty(). We build against S-Lang, so I'll remove the ncurses reference also.

comment:11 follow-up: ↓ 12 Changed 3 weeks ago by IBMJesseG

Andrew, we also made a couple changes to address another subshell issue particular to IBM i. Would you like me to include updated .spec/.patch files in this issue? (code changes are small)

comment:12 in reply to: ↑ 11 Changed 3 weeks ago by andrew_b

Replying to IBMJesseG:

Andrew, the "case $host_os in *os400" is the correct way, since the change is not needed for AIX.

Thanks!

libutil-devel is an IBMi-only package and is needed for the IBM i implementation of openpty().

We also made a couple changes to address another subshell issue particular to IBM i.

We should detect libraries for openpty more generally (see gnulib pty_h and pty modules for the reference). Let's move it to #3915.

Would you like me to include updated .spec/.patch files in this issue? (code changes are small)

No. We have some distro-specific stuff in the source tree (/contrib) but it is orpahed in fact and I don't know who use it.

comment:13 Changed 3 weeks ago by andrew_b

I'm sorry I confused two IBM's operation systems: i and AIX.

comment:14 Changed 3 weeks ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.22

Branch: 3927_ibm_i
changeset:9512e50bb3ece3ef00d4dff11d746c12ec03b113

If you have patches specific to IBM i only, please attach them here.

Last edited 3 weeks ago by andrew_b (previous) (diff)

Changed 3 weeks ago by IBMJesseG

Updated patches and spec file, Sept 2, 2018

comment:15 Changed 3 weeks ago by IBMJesseG

Thanks, Andrew.

To summarize the changes that @jax and I have to contribute:

subshell/common.c changes:

  • openpty() support, for multiple platforms
  • PTY_ZEROREAD fix for IBM i (and potentially others)

configure.ac changes:

  • Change to program paths for perl/ruby/python (very specific to IBM i)
  • utimensat workaround for IBM i
  • define PTY_ZEROREAD for IBM i

And just to clarify the actions you'd like us to take for these:

subshell/common.c changes:

  • openpty() support: bring to #3915
  • PTY_ZEROREAD fix: send to branch 3927_ibm_i

configure.ac changes:

  • Change to program paths: send to branch 3927_ibm_i
  • utimensat workaround for IBM i: send to branch 3927_ibm_i
  • define PTY_ZEROREAD for IBM i : send to branch 3927_ibm_i

When you said to attach the patches "here" I am wasn't sure if you are referring to this issue or the new branch. In any event, I've attached the latest patches and .spec file here for clarity's sake (patches 8sep2018.zip), but will gladly do what is requested.

comment:16 Changed 3 weeks ago by andrew_b

Thanks!

Patches were applied with some my modifications to 3927_ibm_i and 3915_cleanup branches.
Please review and test.

You can checkout these branches from your local repo, or download as archives from Github:
https://github.com/MidnightCommander/mc/archive/3927_ibm_i.zip
https://github.com/MidnightCommander/mc/archive/3915_cleanup.zip

comment:17 Changed 3 weeks ago by andrew_b

3915_cleanup branch doesn't contain i-specific patches. They're in the 3927_ibm_i branch only.

I'm going to merge 3915_cleanup into master during this week.

comment:18 follow-up: ↓ 19 Changed 3 weeks ago by IBMJesseG

Thanks! I've done a review of both branches, and all looks good, except the following add to configure.ac doesn't look like it got applied anywhere.

AC_CHECK_FUNCS(openpty)

I'll let you determine whether you want that conditioned to IBM i, or left as a general check to all platforms (should be ok but I have no means of testing all the necessary platforms).

comment:19 in reply to: ↑ 18 Changed 3 weeks ago by andrew_b

Replying to IBMJesseG:

the following add to configure.ac doesn't look like it got applied anywhere.

AC_CHECK_FUNCS(openpty)

That is in the mc-subshell.m4 file.

comment:20 Changed 3 weeks ago by IBMJesseG

Ahh, sorry I missed that. Thanks!!

comment:21 Changed 2 weeks ago by andrew_b

  • Votes for changeset set to IBMJesseG andrew_b
  • Branch state changed from on review to approved

comment:22 Changed 2 weeks ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from IBMJesseG andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [3f8290e27e70a4f6212e0e0a527c1577e238371e].

git log --pretty=oneline 202b80a..3f8290e

comment:23 Changed 2 weeks ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.