aqa-tests
aqa-tests copied to clipboard
adoptium
external/build_all.sh script is broken
Hi!
I do not understand purpose of https://github.com/adoptium/aqa-tests/blob/master/external/build_all.sh. I addition, it seems it is unused, because - per https://github.com/adoptium/aqa-tests/pull/5553#issuecomment-2346692535 - it was broken since 6.February.2024, and nobody noticed.
of course I may be absolutely missing some point, and despite several quiestions on several places, I never got an reply. Looking forward for explanation!
You seem to have an inconsistent perspective on removing versus keeping files that are not used. You want to remove this one (which was at one point in the past used by someone for local testing), but you argue to keep this other one https://github.com/adoptium/aqa-tests/pull/5553#discussion_r1824272724. I will also note that your tendency to disagree/argue and push back on review requests causes reviewers to not want to review your PRs since you will not make the changes they ask for.
I prefer that we first look at fixing the functionality of build_all.sh, rather than removing the script, and also prefer that every PR is related to an issue that describes the problem along with any discussion regarding potential options for resolving it.
Have you read the linked comment in initial description? The file is broken for an year.
I had clearly written that I will remove it if you insists, but I had kindly asked to reconsider, because the file is quite useful. I'm always doing what reviewer asks for, but not blindly. If I see an reason in that, I try to highlight that. If that is not persuasive enough, I follow what reviwer wanted. You may see that pattern in all my PRs.
I know you want issue for evrey PR, so I'm doing my best to make it. This is an issue.
Also You had promised (sorry, no written evidence) that you will ask other users. "any update" means, if you were able to keep your word, and did what you promised.
I think it is quite common that contributor and reviewer do not agree, and then discussion follows. Of course reviewer have final word, and can always say that it is not discussable . In live projects, where is more active reviewers, it is also quite common that there is more opinions, unluckily, this diversity is missing here.
yes, I looked back at how the file was used previously, thus my answer above "in the past used by someone for local testing" and see the value of being able to generate all files at once (especially if we consider changing how we run these tests in the future by building and caching images and pulling them for testing).
So you really wish to keep broken file, for the unlikely case, that in some future, it may be used again? The file is quite trivial, and in case it will be really for some usage, then it will be quite easy to write it again, and reflect the state of the moment when it is reincarnated.
"I prefer that we first look at fixing the functionality of build_all.sh, rather than removing the script, and also prefer that every PR is related to an issue that describes the problem along with any discussion regarding potential options for resolving it."
It is valid point. I would really like you to ask to all possible users, if it is really used. I doubt, because it is broken so long. The fix is simple - to iterate over versions, or as I put it to https://github.com/adoptium/aqa-tests/pull/5553 to enforce the usage of version. The iteration over versions seems no longer possible, as the support matrix is no longer symmetric. Thus build_all remains bound to https://github.com/adoptium/aqa-tests/pull/5553, which is step allowing to iterate against practically anything. Worst of it all is keeping unused, dead code.
Thanx for the https://github.com/adoptium/aqa-tests/issues/5763! That is very nice info which gives me a bit more info to build on top of. From that point of view I'm definitely ok to close this one. Please do if you wish. I will close on few days time-out if nothing changes.
Changing the title of this issue, so the outcome can be fix the script instead of remove, as needed (based on 5763 enquiry)
Note that it will be "fixed" by https://github.com/adoptium/aqa-tests/pull/5553 's https://github.com/adoptium/aqa-tests/pull/5553/commits/26cd87f7eda6f9521dfa59150dbdfafb4cc225e3