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

Add option to save commit hash in report #917

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

KornevNikita
Copy link
Contributor

This patch adds a command line option for run_conformance_report.py to
save the SYCL-CTS commit hash in the conformance_report.xml.

This option is optional and serves to help to determine the version of
the suite.

This patch adds a command line option for run_conformance_report.py to
save the SYCL-CTS commit hash in the conformance_report.xml.

This option is optional and serves to help to determine the version of
the suite.
@KornevNikita
Copy link
Contributor Author

Example of how the report looks now:
image
image

@bader
Copy link
Contributor

bader commented Jul 26, 2024

@KornevNikita, thanks for working on this.
Proposed approach allows errors with using wrong commit hash. Embedding the Git SHA into the test executable and printing it upon request will be a less error prone approach.

@KornevNikita
Copy link
Contributor Author

@KornevNikita, thanks for working on this. Proposed approach allows errors with using wrong commit hash. Embedding the Git SHA into the test executable and printing it upon request will be a less error prone approach.

My idea is to have the commit hash exactly in the report, so you can determine the version without anything else. I agree, it requires inserting the right hash into the report. But you can simply change it manually (if it's legal to modify the report of course). And anyway the developer should specify the hash in submission_details when submitting a package.

Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

@KornevNikita, thanks for working on this. Proposed approach allows errors with using wrong commit hash. Embedding the Git SHA into the test executable and printing it upon request will be a less error prone approach.

My idea is to have the commit hash exactly in the report, so you can determine the version without anything else. I agree, it requires inserting the right hash into the report. But you can simply change it manually (if it's legal to modify the report of course). And anyway the developer should specify the hash in submission_details when submitting a package.

@KornevNikita, my point is "it's better to add SYCL-CTS commit hash to the submission package automatically (i.e. there should be no manual actions required)". It's easy to use wrong command or make a mistake to provide all information necessary for SYCL-CTS submission.

I'm okay with the idea to add commit hash information to the conformance report. But I think we can do better.

@bader bader requested a review from a team July 29, 2024 16:03
@KornevNikita
Copy link
Contributor Author

But I think we can do better.

@bader agree, but not sure if it can be done automatically. Developers are allowed to pick some version of SYCL-CTS and then cherry-pick some commits from open pull requests if needed. As I understand - in this case the developer is the only one who knows which original hash was used. Not sure how we can automate this case.

@bader
Copy link
Contributor

bader commented Jul 29, 2024

But I think we can do better.

@bader agree, but not sure if it can be done automatically. Developers are allowed to pick some version of SYCL-CTS and then cherry-pick some commits from open pull requests if needed. As I understand - in this case the developer is the only one who knows which original hash was used. Not sure how we can automate this case.

Something like this should do what you want:

git fetch <KhronosGroup/SYCL-CTS>
git mergebase <KhronosGroup/SYCL-CTS>/SYCL-2020
git diff `git mergebase <KhronosGroup/SYCL-CTS>/SYCL-2020` # the output of this command must be added to the submission package (if not empty)

But this is an exceptional situation. I think we expect implementors to use Khronos version of CTS and improve on situations when vanilla version doesn't work.

@bader
Copy link
Contributor

bader commented Sep 5, 2024

@KornevNikita, please, resolve merge conflicts.
@KhronosGroup/sycl-cts-reviewers, please, take a look at this change.

@KornevNikita
Copy link
Contributor Author

I've updated the branch.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Thanks!
It is always a piece of information. It requires to have git and the full repository, but I guess this is manageable in the case there is no git available.

@bader bader merged commit db26da8 into KhronosGroup:SYCL-2020 Sep 10, 2024
8 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.

3 participants