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

Switch expected and actual values in warning for native tasks #231

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

OptimumCode
Copy link
Contributor

@OptimumCode OptimumCode commented Jul 3, 2024

Hi,

I was a bit confused with the current warning when the native benchmark is skipped because the current OS does not match the expected for that target
E.g.:

Skipping benchmarks for 'iosArm64' because they cannot be run on current OS: Expected linux_x64, but was ios_arm64

It sounds more like target iosArm64 expects linux_x64 but the current OS is ios_arm64

If you change the expected and actual values it would be more clear. Please, let me know what you think.

If you disagree feel free to close the PR)

@qurbonzoda qurbonzoda self-requested a review August 14, 2024 19:08
Copy link
Collaborator

@qurbonzoda qurbonzoda left a comment

Choose a reason for hiding this comment

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

I agree that the warning could be confusing. Maybe we can simplify the message to: "Skipping benchmarks for '${target.name}' because they cannot be run on current host '${HostManager.host}'"?

@OptimumCode
Copy link
Contributor Author

Hi, @qurbonzoda. Thank you for the review. I think we should keep the information about the expected OS in the warning. What do you think about this format for the message?

"Skipping benchmarks for '${target.name}' because they cannot be run on current host '${actualHost}' (expected host: '${expectedHost}')"

@qurbonzoda
Copy link
Collaborator

Great! Please apply the change.

@OptimumCode
Copy link
Contributor Author

Hi, @qurbonzoda. I have applied the suggested changes

@qurbonzoda qurbonzoda merged commit a521c0b into Kotlin:master Aug 16, 2024
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.

2 participants