-
-
Notifications
You must be signed in to change notification settings - Fork 249
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
Added top-level comaprator script #3991
Conversation
judovana
commented
Oct 11, 2024
•
edited
Loading
edited
I iwll add license header and do the linter exercise once we agree it is desirbale work to go in. TY! |
PATH="${JAVA_HOME}/bin:${PATH}" bash ./comparable_patch.sh --jdk-dir "${WORKDIR}/${jdkName}" --version-string "$JAVA_VENDOR_VERSION" --vendor-name "$JAVA_VENDOR" --vendor_url "$JAVA_VENDOR_URL" --vendor-bug-url "$JAVA_VENDOR_URL_BUG" --vendor-vm-bug-url "$JAVA_VENDOR_URL_BUG" 2>&1 | tee -a "$LOG" | ||
done | ||
if [ "x${DO_BREAK:-}" == "xtrue" ] ; then | ||
echo "dead" > "${WORKDIR}/${JDK_INFO["first_name"]}/bin/java" |
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.
this looks a little odd? "dead" ? why overwrite what is being compared?
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.
The ${DO_BREAK:-}"
block is serving to tempt your toolchain a bit. In the moment I saw it claims some buiilds to be comparable, I wanted to modify some parts to see if results are as they should be. At the end I left only this two simple modification as placeholder where to do something like that - or use as it is. I do not have hard feelings to remove it, but it was useful.
linter is upset :-) |
Linter is never upset, it just gently reminds! |
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 not sure at the moment we want to add this as another top-level script, which may just complicate things...
Sure. I have no objections to not include this one. But word "another" is interesting. I have not noticed anythig similar around. . I definitely do not want myself, nor anybody else to to keep "supporting" two top level wrappers, but would like to have one which do its job properly. So I would like to adjust the already existing to do what I need, or move workloads which are using the current one to this new one. Can you please pin point me? |
So "another" is actually relative to the "top level use case". We already have in this repo the new AQA rebuild tests here for example:
|
I will close this PR as unnecessary/duplicate, especially as we prefer to use our existing approach and leverage existing AQAvit tools such as TRSS when running and monitoring comparisons and reproducible tests. Also as a reminder, PRs should be related to existing issues which are either in backlog or accepted into a quarterly plan by component owners / PMC / project leads, so we can ensure that we are prioritizing our time and resources well. |
Yah, I have seen that playlist. It did not seems to be doing what I wanted. I will try to look again.
Sure, but there is absiolutely no reason an testsuite could generate more types of results. |
@smlambert Actually one more thing. Three weeks ago I wanted to run compare of one jdk against existing temurins. There was (and afaik still is) nothing which is helping to glue all the parts together to compare existing build against released temurins. Especially under cygwin. |
here you go, #3998 hopefully it will pass through component owners / PMC / project leads |