Improved installer #742
Improved installer #742
Conversation
DevonianTeuchter
requested review from
HarithaIBM,
IgorTodorovskiIBM,
MikeFultonDev and
v1gnesh
as code owners
|
Impressive amount of changes! This will take me a few days to review! |
|
With a minor edit to the last line w.r.t issues closed, we can make GH auto-close them on the merge of this PR. |
My choice of words was deliberate to ensure this didn't happen ;-) As there is a lot of churn and some of the issues are subjective, explicitly checking those issues to see if the submitter is happy with the update is advised. |
|
Investigating test harness failures... |
|
Impressive |
|
I cloned this branch and ran zopen init on my existing file system and got this? Are we able to get it to work with existing file systems? |
|
On a new zopen file system: I have z/OS Open Tools find in my PATH |
I had tested migrating from 0.8.0 and 0.8.2 - but that was using the --refresh rather than re-init options... let me check that migration path
Another thing to investigate... |
|
@IgorTodorovskiIBM I've pushed a further commit that should resolve the issue when running in a new environment (ie. zopen init). slight issue is that when using the checked out version, it will automatically update meta to the latest in the repo so you then have to reapply the clone into the zopen filesystem. No easy way around that as copying the installing meta into the zopen filesystem could leave the system with an older meta than in the repo going forward, depengind on how quickly a packaged release is made available [if that makes sense]. |
|
If anyone has a chance [and is feeling up for a challenge!], the new installer for zopen is available for testing. The updated zopen will generate metadata when first "upgraded" so this needs more than just copying the files across - there needs to be a refresh.
|
|
DevonianTeuchter
force-pushed
the
new_installer
branch
from
2a04431 to
a1fd282
Compare
DevonianTeuchter
force-pushed
the
new_installer
branch
from
a1fd282 to
25cf670
Compare
DevonianTeuchter
force-pushed
the
new_installer
branch
from
6244713 to
6d8b9fe
Compare
DevonianTeuchter
force-pushed
the
new_installer
branch
from
6d8b9fe to
c439bb4
Compare
|
@IgorTodorovskiIBM One of the new commits should resolve your issue with relative local paxes. |
|
To test the PR, replace with fq-path to zopen.test and all instances of zopen.test with test directory> :
|
|
@DevonianTeuchter I would like to test your new installer, but my employer's z/OS system has NO access to github or any other outside network locations and I do not have any copy of git available even if this system did have access to any network locations. If I |
DevonianTeuchter
force-pushed
the
new_installer
branch
from
a64e6bb to
604f5d0
Compare
|
Here's a script I am using to install / update to this new installer:
|
|
barfed right at the end for me: Here's the whole screen without nice colours: |
Oh, maybe I had an older version when I ran it? or Russell fixed it. Do |
DevonianTeuchter
force-pushed
the
new_installer
branch
from
a2d00f3 to
7e21f51
Compare
|
@DevonianTeuchter I gave the latest a test. I think it's getting close! Here's some additional feedback/questions:
Note the -0%, previously it reported “No tests”. Also, sizeDisk and usageQuality are not consistent with the other headers.
|
|
Also, my zopen_releases.json still does not get updated automatically. I need to do this: |
apply shellcheck fixes
|
This is a big one - augment review. Be thorough & systematic pls. |
🤖 Augment PR SummarySummary: Large installer refactor to better support local/offline installs, custom repositories, and transaction-style package operations. Changes:
Technical Notes: Adds/extends config keys (e.g., autocacheclean/autopkgpurge/skip_broken/skip_size_check) and bumps zopen version to 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| @@ -1,4 +1,4 @@ | |||
| #!/bin/sh | |||
| #!/bin/sh -x | |||
There was a problem hiding this comment.
#!/bin/sh -x enables shell tracing by default, which will echo command lines/args (potentially including credentials like PATs/passwords) and makes normal CLI output very noisy. Consider only enabling -x under an explicit --xdebug/env flag (also applies to other scripts in this PR).
🤖 Was this useful? React with 👍 or 👎
| if [ -e "${ZOPEN_PKGINSTALL}/${package}/${package}" ]; then | ||
| printVerbose "Removing main link" | ||
| rm -rf "${ZOPEN_PKGINSTALL}/${package}/${package}" | ||
| if ! rm -rf "${ZOPEN_PKGINSTALL}/${package}/${package}:?"; then |
There was a problem hiding this comment.
This rm -rf "${ZOPEN_PKGINSTALL}/${package}/${package}:?" looks like a typo for the ${var:?} safety expansion; as written it targets a literal .../package:? path and won’t remove the existing symlink. That can leave the old "current" link in place and break zopen alt --set behavior.
🤖 Was this useful? React with 👍 or 👎
| package="$1" | ||
| newver="$2" | ||
| shift | ||
| newver="$1" |
There was a problem hiding this comment.
--set parsing no longer captures the package argument (only newver="$1"), so zopen alt --set <pkg> <ver> will end up swapping/misassigning package and newver (since package is set later in the default branch). This makes setAlt "${package}" "${newver}" unreliable for normal usage.
🤖 Was this useful? React with 👍 or 👎
| # shellcheck disable=SC2034 | ||
| bypassPrereqs=false | ||
| # shellcheck disable=SC2034 | ||
| setActive=true |
There was a problem hiding this comment.
include/common.sh uses $setactive as a boolean command in multiple places (e.g., if $setactive && ...), but this script initializes setActive=true (different variable). If setactive stays unset, it will expand to an empty command and can cause a shell syntax/runtime failure during install (also applies to bin/zopen-upgrade).
🤖 Was this useful? React with 👍 or 👎
| ;; | ||
| "--no-symlink") | ||
| # shellcheck disable=SC2034 | ||
| setactive=true # Do not mesh the package into the file system; leave as stand-alone |
There was a problem hiding this comment.
--no-symlink currently sets setactive=true but never sets nosymlink=true, so the flag appears to have no effect (the install path will still run mergeIntoSystem when nosymlink is false).
🤖 Was this useful? React with 👍 or 👎
| cat <<EOS >"${repoDFile}" | ||
| { | ||
| "type": "$2", | ||
| "metadata_baseurl": "${target}")", |
There was a problem hiding this comment.
The generated repod.json here is invalid JSON due to the extra ) in "metadata_baseurl": "${target}")", (and there’s a similar issue later in the Nexus generateRepoD heredoc where the JSON appears truncated). This will break --genRepoFile/--activate for remote repo types.
🤖 Was this useful? React with 👍 or 👎
| split(values[i], words, /[ \t]+/) | ||
| for (j = 1; j <= length(words); j++) { | ||
| word = words[j] | ||
| printf "Parsed word:>>%s\n", word |
There was a problem hiding this comment.
prettyPrint()’s awk currently prints debug lines like Parsed word:>>... / Started new line:>>... unconditionally, which will pollute stdout for any caller using this formatter. If this is meant for debugging, it likely needs to be gated behind a debug/verbose flag or removed before release.
🤖 Was this useful? React with 👍 or 👎
Update to the installer primarily to handle install direct from file but includes:
-- auto-cache-clean - clean package cache after installation transaction completes [enabled by default]
-- man-db - run man-db automagically after install/alt/remove transactions [enabled if man-db package is installed]
-- caveats - display any "caveats" that a package might have after installation [a caveat being something an end user has to either run, configure or be aware of when using a certain package]
Should resolve/improve: #733 #717 #701 #694 #688 #675 #673 #661 #643 #633 #619 #617 #605 #567 #566 #558 #533 #472 #431 #381 #339 #240