Skip to content

Conversation

jinxidoru
Copy link

This is a pretty small change to the generate-results script that makes it so it'll run on a Mac. The sed command there is slightly different and doesn't like range delete command for some reason. I verified that this new version of the command will also work on gnu sed.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@rschu1ze
Copy link
Member

rschu1ze commented May 1, 2025

@jinxidoru Would you sign the CLA first? Thanks.

@jinxidoru
Copy link
Author

I signed it when I first posted the PR but it does not seem to register the fact that I signed it. Not sure what to do at this point.

@jinxidoru
Copy link
Author

I just tried it again. It had me log in to GitHub then told me that I'd already signed it while redirecting me to GitHub.

@rschu1ze
Copy link
Member

rschu1ze commented May 3, 2025

I tried locally, it looks like we are now generating something different on Linux than before.

@rschu1ze rschu1ze closed this May 3, 2025
@jinxidoru
Copy link
Author

@rschu1ze - Are you sure about that? I just ran it to double check and got identical results. The most recent benchmark commit didn't update the index.html, just some of the results files. So I'm wondering if perhaps what you're seeing is simply the update of the index.html based upon the recent results changes, because I'm not seeing at all how my change could be giving different results on linux.

@rschu1ze
Copy link
Member

rschu1ze commented May 4, 2025

It sounds more appealing to force users to install GNU sed on macOS than having to maintain a lowest-common sed pattern for both versions. I did that in #360.

@rschu1ze rschu1ze mentioned this pull request May 4, 2025
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