-
Notifications
You must be signed in to change notification settings - Fork 167
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
Add IBM i initial playbooks #1923
Conversation
I'm curious, why is Looked over the changes, they look reasonable to me, though whether they are working or not is will have to wait on hooking them up into CI. Any idea how far we are from giving that a try? |
We should be pretty close. We already ran the playbook against our first build system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM insofar I'm able to judge such things, with some suggestions
772a31d
to
bbd4ca7
Compare
@dmabupt thanks, good catch! we hadn't gotten to running jenkins yet because the make test is failing. |
@Trott I'd like to add a |
I believe it's repo-by-repo. |
@@ -67,7 +67,7 @@ | |||
- "!test-softlayer-ubuntu1604-x64-1" | |||
tasks: | |||
- name: remove node and npm packages | |||
when: not os|startswith("win") and not os|startswith("zos") | |||
when: not os|startswith("win") and not os|startswith("zos") and not os|startswith("ibmi") and os != "centos5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there centos5 changes in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not showing because this is still in draft but this PR needs to be rebased onto the current master as it has merge conflicts when I tried to pull down the changes locally. The centos5 stuff was removed in #1984.
ibmi: [ | ||
'gcc', | ||
'gcc-cplusplus', | ||
'gcc-cpp', | ||
'ccache', | ||
'sed-gnu', | ||
'coreutils-gnu', | ||
'zlib-devel', | ||
'python2-pip', | ||
'git', | ||
'ca-certificates-mozilla', | ||
'libstdcplusplus-devel', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To build libuv in the CI we can use either use autoconf/automake or cmake (the gyp based build has been removed from libuv).
65.183.160.52 doesn't have cmake installed. Attempting to build libuv with autoconf/automake yields this:
bash-4.4$ sh autogen.sh
+ libtoolize --copy
libtoolize: error: One of these is required:
libtoolize: gm4 gnum4 m4
libtoolize: error: Please install GNU M4, or 'export M4=/path/to/gnu/m4'.
bash-4.4$
which looks like it requires m4-gnu
bash-4.4$ yum provides m4
ibm | 3.6 kB 00:00:00
ibm/primary_db | 254 kB 00:00:00
m4-gnu-1.4.17-1.ppc64 : GNU m4 macro processor
Repo : ibm
Matched from:
Filename : /QOpenSys/pkgs/bin/m4
ibm/filelists_db | 504 kB 00:00:00
bash-4.4$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardlau feel free to reansible (or hand install) packages on that box if you want to, perhaps to see which build path is easiest, cmake or autotools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using the autotools build. Our IBM i platform support in CMake is not as robust as autotools yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely the autotools build
@ThePrez Looks like we are getting closer to taking another shot at this, do you have time to rebase this and resolve the conflicts? |
Yep. I'm doing several webinars today and tomorrow, but will definitely get this rebased by Friday at the latest. |
Merged and cleaned up |
I rebased this, and force pushed @ThePrez 's branch, and saved the original at https://github.com/sam-github/build/tree/ibmi-orig for future reference. |
@@ -6,3 +6,5 @@ | |||
|
|||
- name: upgrade installed packages | |||
yum: name=* state=latest | |||
# XXX might have needed `use_backend=yum` at end of last line, but prob |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can remove this comment before landing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could break other platforms that need a dnf
/yum4
backend. We may need to condition this by os or set an ansible_pkg_mgr
fact
https://docs.ansible.com/ansible/latest/modules/yum_module.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is about getting ansible to recognise yum
on IBM i I have a one-line patch for ansible to add /QOpenSys/pkgs/bin/yum
to the list of PKG_MGRS
:
--- /home/users/riclau/.local/lib/python2.7/site-packages/ansible/module_utils/facts/system/pkg_mgr.py.old 2020-04-23 12:22:58.779566067 -0400
+++ /home/users/riclau/.local/lib/python2.7/site-packages/ansible/module_utils/facts/system/pkg_mgr.py 2020-04-23 12:24:16.201566043 -0400
@@ -37,6 +37,7 @@
{'path': '/usr/sbin/sorcery', 'name': 'sorcery'},
{'path': '/usr/bin/rpm-ostree', 'name': 'atomic_container'},
{'path': '/usr/bin/installp', 'name': 'installp'},
+ {'path': '/QOpenSys/pkgs/bin/yum', 'name': 'yum'},
]
I'll look at submitting this to ansible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it looks like ansible/ansible#69484 went out in ansible v2.10.0: https://github.com/ansible/ansible/blob/v2.10.1/changelogs/CHANGELOG-v2.10.rst#v2-10-0
I've resolved the merge conflict that arose after I pulled out adding the The ansible scripts work (with my local ansible patched with ansible/ansible#69484) on
After some digging it looks like the
i.e. for some reason
The documentation in this PR updates
@nodejs/platform-ibmi Any pointers on how to resolve the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks close to ready!
There are a couple minor comments, but once addressed, I think this should be merged. Its not set in stone, we can PR more changes later if necessary while getting node.js build jobs working. |
I've compared
Running the playbook gets a bit further now, but errors with
Extracting "msg" and applying the line breaks:
I've fixed the error about the bad id by moving |
Fixed the broken packages by manually running
so that only
|
I've used the current version of this PR to set up https://ci.nodejs.org/computer/test-iinthecloud-ibmi72-ppc64_be-2/ and tested that it can successfully run the libuv builds, so I think barring the documentation/comment issues @sam-github has raised this is good to go as a starting point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending the points raised by @sam-github.
Co-authored-by: Sam Roberts <vieuxtech@gmail.com>
Co-authored-by: Sam Roberts <vieuxtech@gmail.com>
Co-authored-by: Sam Roberts <vieuxtech@gmail.com>
I think I addressed the comments, plus some other minor adds/cleanup |
🎂 |
Supercedes and addresses PR #1700
Addresses part of issue #1697
TODO items remaining are (at least):
su
works now.