Skip to content
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

Made both CryptoTests_jtreg and CryptoTests targets vendor neutral #5249

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

judovana
Copy link
Contributor

It is passing for all vendors, the differences lies in underlyng OS, not JDK

It is passing for all vendors, the differences lies in underlyng OS, not JDK
@judovana judovana marked this pull request as draft April 19, 2024 15:54
@judovana
Copy link
Contributor Author

However, as pointed out in #5248 (review) they must have no intersection on platforms/OSes

I would like to return CryptoTest target to be vendor neutral, and correclty run on platforms where it can.
On all other platforms - or generally anywhere where @llxia needs it - it should run the new CryptoTests_jtreg

Can you help me to set up platforms properly or is this out of cope?

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a disable tag to the CryptoTests target at line 38

<disable>
	<comment>This target is for those using jtreg-plugin, and wanting the extra functionality provided by run.sh, tarring of results, downloading of bespoke jtreg.jar</comment>
</disable>

That way you can run it with _disabled.CryptoTests and others will not be running the test material twice.

@judovana
Copy link
Contributor Author

judovana commented Apr 22, 2024

Thanx, that is already much more friendly, Will continue with it. TY!

<comment>
This target is for those who would like to run upstream fully supported way of how to use this suite.
This launcher runs in GH, can enable custom jtreg.jar with upstreamed but unreleased stdout fixes,
can correctly enable/disable/setup kerberos tests and works in any CWD.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is inappropriate, please fix it with the suggested sentence instead of using loaded terms and phrases. For the record, it is not only for the issue on s390x that others do not want to use run.sh, it is precisely because it has a bunch of extra behaviour that is unneeded and unwanted by those using AQAvit Jenkins pipelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will try to elaborate on it. However there is no single word lying. Maybe except ancient s390x platforms . Although the s390x was the only platform I was able to get close reproduce the issue. On MACs it was working fine.

I can not agree that ability to enable the kerberos based thest is unwanted. The only disputable part was tarring of jtr.xml files, where I offered that I will add support for both.
Similarly I think that any possible user should be warned that run.sh is what is used in git hub actions.

As I stated before, I would much more likely create the original target with the original launcher to be usefull for anybody (and I committed myself to it) then do any forking. But it have been already decided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fact-based:
<comment>This target is for those using GH actions or the Jenkins jtreg-plugin. The run.sh script provides extra functionality, tarring of results, downloading of bespoke jtreg.jar from a particular untagged commit SHA that is not yet upstreamed to a release version of jtreg that is used to set up some kerberos tests.</comment>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fact-based: <comment>This target is for those using GH actions or the Jenkins jtreg-plugin. The run.sh script provides extra functionality, tarring of results, downloading of bespoke jtreg.jar from a particular untagged commit SHA that is not yet upstreamed to a release version of jtreg that is used to set up some kerberos tests.</comment>

Thank you and sorry for being nit picky about that. It i ok by me, except the tail where is error.

-is not yet upstreamed to a release version of jtreg that is used to set up some kerberos tests
+is not yet upstreamed to a release version of jtreg that is used see complete output in jtr.xml files. In addition it simplify enable/disable of kerberos-based tests which needs remote, pre-set KDC.  

wdyt?

@judovana judovana force-pushed the cryptoTestsVendorNeutral branch from 5ebae65 to 1717974 Compare April 23, 2024 12:59
@judovana judovana force-pushed the cryptoTestsVendorNeutral branch from 1717974 to 743b852 Compare April 23, 2024 13:20
Co-authored-by: Shelley Lambert <slambert@gmail.com>
@karianna karianna requested a review from smlambert April 24, 2024 07:59
@judovana
Copy link
Contributor Author

Thanx a lot for patience! Ok by me if I have permissions to say anything to it. TY!

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still in draft mode. I can move it out to merge @judovana if you have made your final edits.

@judovana judovana marked this pull request as ready for review April 25, 2024 13:45
@judovana
Copy link
Contributor Author

Thanx, can we mere now please?

@llxia llxia merged commit af78d25 into adoptium:master Apr 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants