Closed netskink closed 7 months ago
This might be a difference between /bin/sed and our sed so to be safe you may want to also say /bin/sed ?
Hmm, I was just making a change to all the sed occurances in the file and going to make it ~/zopen/usr/local/bin/sed
instead. This kind of distracts and not ideal. Then I thought I should rather do something like get the location of the script and set the path to be relative to there.
ie hermetic build thing.
Then I thought I should clarify with you, how do you want to do that? Or, perhaps do in a follow up issue. ie. this patch fixes the zopen install findutilsport problem where the strip port does nto work. Then do a separate patch to ensure sed is from zopen rather than USS default sed.
If I file as an issue, I can always revisit. I'm kind of thinking gnport is higher priority.
This might be a difference between /bin/sed and our sed so to be safe you may want to also say /bin/sed ?
Hmm, I was just making a change to all the sed occurances in the file and going to make it
~/zopen/usr/local/bin/sed
instead. This kind of distracts and not ideal. Then I thought I should rather do something like get the location of the script and set the path to be relative to there.ie hermetic build thing.
Then I thought I should clarify with you, how do you want to do that? Or, perhaps do in a follow up issue. ie. this patch fixes the zopen install findutilsport problem where the strip port does nto work. Then do a separate patch to ensure sed is from zopen rather than USS default sed.
If I file as an issue, I can always revisit. I'm kind of thinking gnport is higher priority.
Now that we rely on the zopen_releases.json cache (which doesn't have a port) suffix, the code to handle projects ending with port was removed. I'm fine with adding that support back in.
Do you get different results with zopen sed vs /bin/sed? The regex doesn't seem overly complex here so I doubt it will differ. We would we need to embed sed as we do with curl and jq if we wanted to go this route.
fwiw, this is the code in zopen-install
as it originally was:
printVerbose "Stripping any version (%), tag (#) or port suffixes"
toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#.*port##')
# sed can use # as a delimter rather than normal / delimiter.
toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#port##')
and then later this in there
# why strip if going to add as is anyway?
# ORIG installArray="${installArray} ${chosenRepo}"
installArray="${installArray} ${toolfound}"
I don't know all the variants for testing installs. (Do we have a unit test for install options?). So I was conjecturing in the comments above. The second test in the original comment shows that the .*port is not stripping "port", but instead stripping all text ending in port. Consquently, why I simply removed the match all bit.
In the second stanza, it seems its another bug. Its "stripping" text, but then the installarray adds the original non stripped entry. I was thinking the point of this code is to strip info and then add the stripped version to the list of repo's to install.
Based upon the comments I'm doubly confused now.
Is this dead code which should be removed and the place to make the mod is in a different file and it has differernt logic?
What are the various input forms that are possible with zopen install?
zopen install foo
<- the only one I knnow which is validzopen install fooport
<- realize the repo name is not how the install mechanism works and strip automatically to:
zopen install foo
zopen install foo=1.0
<- install a specific version of foo. perhaps this code adds to install array foo, but maninpulates aversion array somewhere?zopen install fooport=1.0%sometag
fwiw, this is the code in
zopen-install
as it originally was:printVerbose "Stripping any version (%), tag (#) or port suffixes" toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#.*port##') # sed can use # as a delimter rather than normal / delimiter. toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#port##')
and then later this in there
# why strip if going to add as is anyway? # ORIG installArray="${installArray} ${chosenRepo}" installArray="${installArray} ${toolfound}"
I don't know all the variants for testing installs. (Do we have a unit test for install options?). So I was conjecturing in the comments above. The second test in the original comment shows that the .*port is not stripping "port", but instead stripping all text ending in port. Consquently, why I simply removed the match all bit.
In the second stanza, it seems its another bug. Its "stripping" text, but then the installarray adds the original non stripped entry. I was thinking the point of this code is to strip info and then add the stripped version to the list of repo's to install.
Based upon the comments I'm doubly confused now.
- Is this dead code which should be removed and the place to make the mod is in a different file and it has differernt logic?
What are the various input forms that are possible with zopen install?
zopen install foo
<- the only one I knnow which is valid
zopen install fooport
<- realize the repo name is not how the install mechanism works and strip automatically to:
zopen install foo
zopen install foo=1.0
<- install a specific version of foo. perhaps this code adds to install array foo, but maninpulates aversion array somewhere?`zopen install foo%sometag <- ditto?
are combos possible? *.
zopen install fooport=1.0%sometag
Nope, not dead code - it's an initial sanity check that what was specified on the command line correlates to a valid package name before handing it off to more specialized processing later
As your examples show, specifying on the cli to install findutil, findutilport, findutil%atag or findutil=v.r.m.f should [currently] all be valid, with the code you identified supposed to strip it back to "findutil" as an initial sanity check that the package specified is actually valid. Inside handlePackageInstall, the logic for actually handling tagged and versioned information needs the suffix still, hence why the "unstripped" version is passed through. However... the initial code that you fixed for the sed line does [currently] need fixed as it matches and strips everything with the expression s#.*port##
- but using the suggested s#port##
might replace the first instance of port it finds; if ever there is a package such as "exporterport", that'll end up looking like "exterport"; that sed expression should really be s#port$##
, s'\(.*\)port$#\1#
or equivalent shell expansion or... with the addition of a -n $toolfound
-type check as well in case of specifying zopen install port
or zopen install =1.2.3.4
for example.
*That being said... as @IgorTodorovskiIBM mentions earlier, the port suffix was dropped from the actual naming of a package so we shouldn't really need to do any stripping of it these days [the version/tag will still be needed at this point]. If a user specifies 'pkgport', it should get rejected as an invalid package, with 'pkg' being the required install parameters. It would solve any issues if ever there was package called something like "export" or "thisport" - so just treat the input as the proper package and not worry about 'port' suffixes.
And as for unit testing, there is a framework that Mike put together that we are adding to, but a lot of testing of this type of thing is manual at the moment.
[hopefully that makes any sense!]
Nope, not dead code - it's an initial sanity check that what was specified on the command line correlates to a valid package name before handing it off to more specialized processing later As your examples show, specifying on the cli to install findutil, findutilport, findutil%atag or findutil=v.r.m.f should [currently*] all be valid, with the code you identified supposed to strip it back to "findutil" as an initial sanity check that the package specified is actually valid. Inside
The santity check before continuing makes sense. That is why you strip, verify the strip is in the list, but then add back the non stripped to the list of install packages.
handlePackageInstall, the logic for actually handling tagged and versioned information needs the suffix still, hence why the "unstripped" version is passed through. However... the initial code that you fixed for the sed line does [currently] need fixed as it matches and strips everything with the expression `s#.port##
- but using the suggested
s#port##might replace the first instance of port it finds; if ever there is a package such as "exporterport", that'll end up looking like "exterport"; that sed expression should really be
s#port$##,
s'(.*)port$#\1#`
That is a good point. I'll update the diff to reflect that port could be anywhere in the string and only remove the last entry.
or equivalent shell expansion or... with the addition of a
-n $toolfound
-type check as well in case of specifyingzopen install port
orzopen install =1.2.3.4
for example.
This I'm unclear on. Possibly I understand some portion of it. If a if [ -n $toolfound ]
its a nonzero string. You want to check for some particular value? Compare against what? The list of known repos?
*That being said... as @IgorTodorovskiIBM mentions earlier, the port suffix was dropped from the actual naming of a package so we shouldn't really need to do any stripping of it these days [the version/tag will still be needed at this point]. If a user specifies 'pkgport', it should get rejected as an invalid package, with 'pkg' being the required install parameters. It would solve any issues if ever there was package called something like "export" or "thisport" - so just treat the input as the proper package and not worry about 'port' suffixes.
Ok, so in this case, the conclusion is don't worry about stripping, simply look for the name and if its not a proper repo name, tell them during the sanity check. ie.
zopen install findutilsport
return
package name not found
, despite that being the actual name of the repo.
And as for unit testing, there is a framework that Mike put together that we are adding to, but a lot of testing of this type of thing is manual at the moment.
Having tests would be a great help in this situation.
[hopefully that makes any sense!]
Yes, DT, it helps.
sounds great I’ll work on that with Igor tomorrow
Here's a simple one for update-cacert: https://github.com/ZOSOpenTools/metaport/pull/16/files. @DevonianTeuchter has added a couple as well recently
Thanks. I’ll use that as a guide.
/run tests
@DevonianTeuchter @MikeFultonDev are you ok with this?
@netskink looks like there's a conflict in /bin/zopen-install
I’ll address on Monday. Thanks for the update
@netskink , regarding @DevonianTeuchter's comment here:
The code here is meant to do an up-front check to make sure that the port prefix (whether followed by a version, tag or "port") is valid. Further processing within handlePackageInstall uses that additional information for searching for versioned/tagged releases so stripping that now [ie passing toolrepo] will break that processing later. The actual fix needs to be in handlePackageInstall() name=$(echo "${fullname}" | sed -e 's#[=%].*##') repo="${name}" <- this should have the "${name%port}" expansion The twin parsing is so that the list of supplied packages can be parsed first; if the check is only done in the second instance, then you might have a number of packages specified and it would only be after installing all the previously "well specified" packages that you'd find the broken one.
I think what we want here is an upfront error when installing multiple ports. So for example:
zopen install git findutilsport
Will it install git first and then print the about findutilsport having the port suffix?
@netskink , regarding @DevonianTeuchter's comment here:
The code here is meant to do an up-front check to make sure that the port prefix (whether followed by a version, tag or "port") is valid. Further processing within handlePackageInstall uses that additional information for searching for versioned/tagged releases so stripping that now [ie passing toolrepo] will break that processing later. The actual fix needs to be in handlePackageInstall() name=$(echo "${fullname}" | sed -e 's#[=%].*##') repo="${name}" <- this should have the "${name%port}" expansion The twin parsing is so that the list of supplied packages can be parsed first; if the check is only done in the second instance, then you might have a number of packages specified and it would only be after installing all the previously "well specified" packages that you'd find the broken one.
I think what we want here is an upfront error when installing multiple ports. So for example:
zopen install git findutilsport
Will it install git first and then print the about findutilsport having the port suffix?
I don't know if it will handle multiple install packages. To be honest I never tired it. I just tried to do things like this:
$ zopen install foo
$ zopen install fooport
The original code never seemed to match. The subsquent comparison clause was never hit. ie if x==y where y was the result of the sed, was always blank for y.
FWIW, I removed the code in the clause and moved it to the top of the file since you said the first check was a sanity check. What remains at the bottom was based upon your request to collect the matching group.
I can test to see what multiple install packages do, but to be honest I believe this change is way over due and beyond the original objective. I propose we merge this one and raise a new issue. Fix one issue at a time and move on. I'm all for collaboration and am happy to adjust accordingly. Please don't let my ability to talk be taken negative. I just want to drive this to completion.
Using the patch plus some debug code:
for chosenRepo in $(echo "${chosenRepos}" | tr ',' '\n'); do
printVerbose "Processing repo: ${chosenrepo}"
printVerbose "Stripping any version (%), tag (#) or port suffixes"
+ echo "chosenRepo: $chosenRepo"
toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#\(.*\)port$#\1#')
+ #toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#.*port$##')
+ echo "toolrepo: $toolrepo"
printVerbose "Adding prefix and 'port' suffix" # quicker than testing for presence!
toolfound=$(echo "${repo_results}" | awk -vtoolrepo="${toolrepo}" '$0 == toolrepo {print}')
if [ "${toolfound}" = "${toolrepo}" ]; then
+ # also that is an = above and not a ==. I'm not sure what the intention is
+ echo "hitting the toolfound vs toolrepo check"
printVerbose "Adding '${chosenRepo}' to the install queue"
installArray="${installArray} ${chosenRepo}"
printVerbose "Removing valid port from input list"
Try with port suffix added
$ zopen install findutilsport
- Querying remote repo for latest package information
chosenRepo: findutilsport
toolrepo: findutils
hitting the toolfound vs toolrepo check
Installing package: findutilsport
Usage error: please install using base project name without port suffix.
try: zopen install findutils
Try with just repo name specified
$ zopen install findutils
- Querying remote repo for latest package information
chosenRepo: findutils
toolrepo: findutils
hitting the toolfound vs toolrepo check
Installing package: findutils
- Replacing findutils version '4.9.0.20231109_042435' with '4.9.0.20231115_215243'
- Found existing file 'findutils-4.9.0.20231115_215243.zos.pax.Z' in zopen package cache
- Processing findutils-4.9.0.20231115_215243.zos.pax.Z...
After this operation, 5.14 MB of additional disk space will be used.
Do you want to continue? [y/n]
Trying with some unknown repo and port suffix
$ zopen install fooport
- Querying remote repo for latest package information
chosenRepo: fooport
toolrepo: foo
***ERROR: The following requested port(s) do not exist:
fooport
Trying with some unknown repo
$ zopen install foo
- Querying remote repo for latest package information
chosenRepo: foo
toolrepo: foo
***ERROR: The following requested port(s) do not exist:
foo
for chosenRepo in $(echo "${chosenRepos}" | tr ',' '\n'); do
printVerbose "Processing repo: ${chosenrepo}"
printVerbose "Stripping any version (%), tag (#) or port suffixes"
- toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#\(.*\)port$#\1#')
+ echo "chosenRepo: $chosenRepo"
+ #toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#\(.*\)port$#\1#')
+ toolrepo=$(echo "${chosenRepo}" | sed -e 's#%.*##' -e 's#=.*##' -e 's#.*port$##')
+ echo "toolrepo: $toolrepo"
printVerbose "Adding prefix and 'port' suffix" # quicker than testing for presence!
toolfound=$(echo "${repo_results}" | awk -vtoolrepo="${toolrepo}" '$0 == toolrepo {print}')
if [ "${toolfound}" = "${toolrepo}" ]; then
+ # also that is an = above and not a ==. I'm not sure what the intention is
+ echo "hitting the toolfound vs toolrepo check"
printVerbose "Adding '${chosenRepo}' to the install queue"
installArray="${installArray} ${chosenRepo}"
printVerbose "Removing valid port from input list"
Try with port suffix added
$ zopen install findutilsport
- Querying remote repo for latest package information
chosenRepo: findutilsport
toolrepo:
hitting the toolfound vs toolrepo check
Installing package: findutilsport
Usage error: please install using base project name without port suffix.
try: zopen install findutils
Try with just repo name specified
$ zopen install findutils
- Querying remote repo for latest package information
chosenRepo: findutils
toolrepo: findutils
hitting the toolfound vs toolrepo check
Installing package: findutils
- Replacing findutils version '4.9.0.20231109_042435' with '4.9.0.20231115_215243'
- Found existing file 'findutils-4.9.0.20231115_215243.zos.pax.Z' in zopen package cache
- Processing findutils-4.9.0.20231115_215243.zos.pax.Z...
After this operation, 5.14 MB of additional disk space will be used.
Do you want to continue? [y/n]
Trying with some unknown repo and port suffix
$ zopen install fooport
- Querying remote repo for latest package information
chosenRepo: fooport
toolrepo:
hitting the toolfound vs toolrepo check
Installing package: fooport
Usage error: please install using base project name without port suffix.
try: zopen install foo
Trying with some unknown repo
$ zopen install foo
- Querying remote repo for latest package information
chosenRepo: foo
toolrepo: foo
***ERROR: The following requested port(s) do not exist:
foo
I'm not certain what the first clause is doing, but without the group, the toolrepo is mostly blank. I thought this would at least provide a response.
I looked to verify the if = clause check vs if ==, this is indeed the original code. I'm not certain if its valid or not. If its invalid or needs comment, i propose we move to a new issue. See here
With the patch in pull request present:
$ zopen install findutils git
- Querying remote repo for latest package information
chosenRepo: findutils
toolrepo: findutils
hitting the toolfound vs toolrepo check
chosenRepo: git
toolrepo: git
hitting the toolfound vs toolrepo check
Installing package: findutils
- Replacing findutils version '4.9.0.20231109_042435' with '4.9.0.20231115_215243'
- Found existing file 'findutils-4.9.0.20231115_215243.zos.pax.Z' in zopen package cache
- Processing findutils-4.9.0.20231115_215243.zos.pax.Z...
After this operation, 5.14 MB of additional disk space will be used.
Do you want to continue? [y/n]
$ zopen install findutils git
- Querying remote repo for latest package information
chosenRepo: findutils
toolrepo: findutils
hitting the toolfound vs toolrepo check
chosenRepo: git
toolrepo: git
hitting the toolfound vs toolrepo check
Installing package: findutils
- Replacing findutils version '4.9.0.20231109_042435' with '4.9.0.20231115_215243'
- Found existing file 'findutils-4.9.0.20231115_215243.zos.pax.Z' in zopen package cache
- Processing findutils-4.9.0.20231115_215243.zos.pax.Z...
After this operation, 5.14 MB of additional disk space will be used.
Do you want to continue? [y/n]
$ zopen install findutilsport gitport
- Querying remote repo for latest package information
chosenRepo: findutilsport
toolrepo: findutils
hitting the toolfound vs toolrepo check
chosenRepo: gitport
toolrepo: git
hitting the toolfound vs toolrepo check
Installing package: findutilsport
Usage error: please install using base project name without port suffix.
try: zopen install findutils
(Note the check at top is still present, just the regex with sed grouping is not)
$ zopen install findutilsport gitport
- Querying remote repo for latest package information
chosenRepo: findutilsport
toolrepo:
hitting the toolfound vs toolrepo check
chosenRepo: gitport
toolrepo:
hitting the toolfound vs toolrepo check
Installing package: findutilsport
Usage error: please install using base project name without port suffix.
try: zopen install findutils
Igor, regarding:
Just to confirm, if you do this:
zopen install --reinstall git findutilsport
will it install git first and then complain about findutilsport?
Yes.
$ zopen install --reinstall git findutilsport
- Querying remote repo for latest package information
chosenRepo: git
toolrepo: git
hitting the toolfound vs toolrepo check
chosenRepo: findutilsport
toolrepo: findutils
hitting the toolfound vs toolrepo check
Installing package: git
- Replacing git version '2.41.0.20231023_154410' with '2.43.0.20231127_145951'
- Downloading git-2.43.0.20231127_145951.zos.pax.Z file from remote to zopen package cache...
- Downloaded git-2.43.0.20231127_145951.zos.pax.Z file from remote to zopen package cache.
- Processing git-2.43.0.20231127_145951.zos.pax.Z...
After this operation, 106.41 MB of additional disk space will be used.
Do you want to continue? [y/n]
y
- Expanding git-2.43.0.20231127_145951.zos.pax.Z
- Expanded
- Integration complete
- Checking for env file
- .env file found, adding to profiled processing
- Sourcing environment to run any setup
Setting up git...
Setup completed.
- Checking for obsoleted files in /z/jd895801/zopen/usr/ tree from git
- Checking 604 potential links
- Complete
- Checking 73 dir links
Successfully installed git
Installing package: findutilsport
Usage error: please install using base project name without port suffix.
try: zopen install findutils
Igor, regarding:
Just to confirm, if you do this:
zopen install --reinstall git findutilsport
will it install git first and then complain about findutilsport?
Yes.
$ zopen install --reinstall git findutilsport - Querying remote repo for latest package information chosenRepo: git toolrepo: git hitting the toolfound vs toolrepo check chosenRepo: findutilsport toolrepo: findutils hitting the toolfound vs toolrepo check Installing package: git - Replacing git version '2.41.0.20231023_154410' with '2.43.0.20231127_145951' - Downloading git-2.43.0.20231127_145951.zos.pax.Z file from remote to zopen package cache... - Downloaded git-2.43.0.20231127_145951.zos.pax.Z file from remote to zopen package cache. - Processing git-2.43.0.20231127_145951.zos.pax.Z... After this operation, 106.41 MB of additional disk space will be used. Do you want to continue? [y/n] y - Expanding git-2.43.0.20231127_145951.zos.pax.Z - Expanded - Integration complete - Checking for env file - .env file found, adding to profiled processing - Sourcing environment to run any setup Setting up git... Setup completed. - Checking for obsoleted files in /z/jd895801/zopen/usr/ tree from git - Checking 604 potential links - Complete - Checking 73 dir links Successfully installed git Installing package: findutilsport Usage error: please install using base project name without port suffix. try: zopen install findutils
Ok, thanks for verifying. Let's open an issue to track that. I'll merge this PR in the meantime.
Thanks for merging. Good idea on a new PR. Please assign to me since I don’t quite follow what is the follow up problem. A fresh issue should convey the remaining issue.
This is a fix for this bug https://github.com/ZOSOpenTools/meta/issues/567
It appears the original code was attempting to strip strings but it did not work. Here is some explantion.
@MikeFultonDev @DevonianTeuchter