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

Generated benchmark threshold files may not be Windows compatible #308

Open
slashmo opened this issue Feb 9, 2025 · 2 comments
Open

Generated benchmark threshold files may not be Windows compatible #308

slashmo opened this issue Feb 9, 2025 · 2 comments

Comments

@slashmo
Copy link

slashmo commented Feb 9, 2025

Context

We received an issue in Swift Distributed Tracing (apple/swift-distributed-tracing#165) about not being able to check out the library on Windows. Upon inspection, we found the issue to be the file names of some of our Benchmark thresholds. In our case, the Benchmark names contained a : which isn't valid in file paths on Windows.

Solution

For now, we'll simply fix this problem by replacing the colons in our Benchmark names. I was wondering though if longterm this could be prevented by package-benchmark in the first place to avoid folks from running into similar issues. One option that came to mind is to hash the Benchmark name and use that hash as file name. This would come at the cost of not being able to easily navigate around threshold files but I'm not sure if that's a valid concern. What do you think?

@hassila
Copy link
Contributor

hassila commented Feb 11, 2025

Yeah, this is related to #193 really - we'd definitely want to do that, but perhaps what you suggest with using a hash of the file name would be ok in practice (modulo hash collisions, but one could easily construct collisions with any name mangling algorithm too) - just a simple check that there are no collisions and otherwise warn would be more than ok in practice I think.

@slashmo
Copy link
Author

slashmo commented Feb 11, 2025

just a simple check that there are no collisions and otherwise warn would be more than ok in practice I think.

Yep, that would work too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants