-
-
Notifications
You must be signed in to change notification settings - Fork 102
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
AixPB: Put /usr/bin at the front of PATH for AIX #2288
Conversation
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.
@Haroon-Khel Have we tested this on any of the machines with some builds and test jobs to ensure nothing breaks as a result if picking up the AIX system tools by default? If so I'm happy to merge.
See also #1979 which adjusts the |
This pr was tested in #2057 (comment) |
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'm ok with this, but we need a plan for updating the existing machines as well since I don't think these regexs will adjust the existing machines.
:) |
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.
OK, I have been playing around with how to do this in an idempotent way. See this as a template for what the change could be.
- key point is to 'slurp' the file, search the shells variable for bash and append it, if it is missing - do nothing if it already exists.
# Playbook to test slurp module/builtin
---
- hosts: localhost
gather_facts: no
pre_tasks:
- name: Examine login.cfg
slurp:
src: /etc/security/login.cfg
register: logincfg
- name: extract shells setting in login.cfg
set_fact:
shells: "{{ logincfg['content'] | b64decode | regex_search('shells = (.*)') }}"
- name: check for bash in shells setting
set_fact:
bash_in_shells: "{{ shells.find('bash') }}"
tasks:
- name: "Add bash to shells variable, if missing."
block:
- name: "Add bash to shells variable, if missing."
replace:
path: /etc/security/login.cfg
regexp: '(shells = [^ \n]+)'
replace: '\1,/usr/bin/bash,/bin/bash'
tags: login_shell
- name: "Add bash to /etc/shells"
blockinfile:
path: /etc/shells
block: |
/usr/bin/bash
/bin/bash
tags: login_shell
when: bash_in_shells == "-1"
Just occurred to me - why do we need |
@Haroon-Khel Is this ready to go in? Have you put together such a plan yet, and if not can we make sure this is done please. |
|
You want a plan? Just say do it - and -
|
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.
- remove /opt/freeware/bin from PATH.
- having /opt/freeware/bin in, or not in, the PATH seems to have no effect on jenkins jobs (see adopt04 PATH).
- my expectation is that the build commands going to jenkins are modifying the PATH variable anyway (e.g., as for xlc16 jobs).
- Better to ALWAYS rely on the build-scripts to setup/verify the environment variables - rather than expect a variable to be provided by /etc/environment and/or /etc/profile
e.g., the current Adoptium/AIX PATH variable definitions:
[email protected]:[/home/root]dsh-adopt.ksh "echo \$PATH"
adopt01:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java6/jre/bin:/usr/java6/bin
==============
adopt02:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java6/jre/bin:/usr/java6/bin
==============
adopt03:
/usr/bin:/opt/IBM/xlC/13.1.3/bin:/opt/freeware/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java8_64/jre/bin:/usr/java8_64/bin:/opt/bin
==============
adopt04:
/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java7_64/jre/bin:/usr/java7_64/bin
==============
adopt05:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java6/jre/bin:/usr/java6/bin
==============
adopt06:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java6/jre/bin:/usr/java6/bin:/opt/bin
==============
adopt07:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java6/jre/bin:/usr/java6/bin:/opt/bin
==============
adopt08:
/opt/freeware/bin:/opt/IBM/xlC/13.1.3/bin:/usr/bin:/etc:/usr/sbin:/usr/ucb:/usr/bin/X11:/sbin:/usr/java8_64/jre/bin:/usr/java8_64/bin:/opt/bin
==============
Hmmm in what sense is bash explcitly needed by |
Implementing this is going to have to wait at the moment - we've got a release starting next week and I don't want to risk any stability issues. |
ok. |
|
|
You can leave this open, but as I am re-writing most of these things - and even have a plan for not needing to modify PATH in /etc/profile - I recommend - DO Not merge. |
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.
Actually, my request is to not do anything, better, close the PR and tag won't fix' or something like that.
We are a year along, nearly, and I have a way to do this without modifying the global PATH environment variable.
@aixtools what is your current planned solution? I believe we currently run into problems if the default |
a) use this to get back onto the program.
|
I'm wondering if we can get away without that. In the build scripts we have https://github.com/adoptium/temurin-build/blob/feb021cc605840022713485062a7cebe6f4be9b0/configureBuild.sh#L281 which should make it invoke Which leaves the test jobs. There is already some handling of this in https://github.com/adoptium/aqa-tests/blob/b60d63cdff92c0ef8079cb5a537ceca28df81b42/buildenv/jenkins/JenkinsfileBase#L429 to force it to invoke @smlambert as far as you're aware does this mean there should no longer be a dependency on @aixtools If we can verify that some examples of openjdk/sanity/perf runs run ok on the machines with the AIX make in the path first and |
Based on the absence of JDK21: https://ci.adoptium.net/job/build-scripts/job/jobs/job/jdk21u/job/jdk21u-aix-ppc64-temurin/63/console |
what I recall as the proposed solution was to add PATH to the client environment - via a jenkins parameter. Then the PATH does not impact system administration. |
Yep. I think some of the scripts are probably explicitly adding it anyway so it may not even need to be across the whole of the jenkins client environment. |
The JDK21 job above seems to be running through ok on the machine without /opt/freeware/bin at the start of the path however the JDK8 one is less happy:
There is some logic in https://github.com/adoptium/temurin-build/blame/7dc73f3a9a78fee748c34be720193b50ea30cbbb/sbin/prepareWorkspace.sh#L649 which explicitly uses the keytool from the boot JDK. It looks like the above error occurs if this is set to a J9VM which is is on some machines such as adopt04 (test-aix-2) which is causing this problem. From the command line I was able to download a Temurin 8 into a temporary location to use as a boot JDK (consistent with the build machines) and it ran through ok :-) |
@Haroon-Khel will need a rebase |
@Haroon-Khel Do you think we can get this one in soon? That will give us a bit of runway before the next set of releases. |
Uh using github's tool to rebase did not work out as I expected 😅 |
5e521fd
to
feeaa2a
Compare
Continuing this in #3716 |
Copy of #2057, but that wont merge due to the owner of that pr not having eclipse authorisation.
I've also removed the
/etc/shells
addition from #2057 since it was unrelated to the initial reason for the pr and was preventing it from being merged. It can be added later in another prThe changes here have been tested already in #2057