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

Make Paths Windows‐Compatible #9

Merged
merged 5 commits into from May 12, 2021
Merged

Make Paths Windows‐Compatible #9

merged 5 commits into from May 12, 2021

Conversation

SDGGiesbrecht
Copy link
Contributor

This resolves swift‐collections‐benchmark’s portion of apple/swift-collections#33.

Checklist

  • I've read the Contribution Guidelines
  • My contributions are licensed under the Swift license.
  • I've followed the coding style of the rest of the project.
  • I've added tests covering my changes (if appropriate).
    • I have included a Windows GitHub action that will catch regressions, but I can remove it again if you would prefer.
  • I've verified that my change compiles and works correctly, and does not break any existing tests.
    • I searched for the filenames across the project and hopefully found and changed everything that pointed at them.
    • I do not know whether these identifiers refer to the images or not, or if they are user‐facing. It also is not clear to me whether they are reused in generating documentation, or if they are just an archived example. Let me know what you want done with them.
  • [N/A] I've updated the documentation (if appropriate).

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

These file names were generated by the tool itself, based on the corresponding library definition, which has been updated to use - instead of :, and we should use the same names here.

To prevent similar issues when people try to run benchmarks on Windows, it would be a good idea to make sure the tool filters out invalid characters from the filenames it generates. The filename sanitizer code is currently really dumb, and it's not even factored out. I like the idea of systematically replacing /, \\, :, >, \0 etc with underscores, depending on the current system.

(It needs to be possible to use colons and angle brackets in chart titles, even if the underlying filesystem forbids their use.)

.github/workflows/Windows.yaml Outdated Show resolved Hide resolved
Documentation/01 Getting Started.md Outdated Show resolved Hide resolved
Documentation/01 Getting Started.md Outdated Show resolved Hide resolved
@SDGGiesbrecht
Copy link
Contributor Author

These file names were generated by the tool itself, based on the corresponding library definition, which has been updated to use - instead of :, and we should use the same names here.

L.O.L. I was trying to work on the dependency graph from the bottom up (swift-collections-benchmarks before swift-collections), but I guess I was holding it backwards. With the original definitions changed, will all the derived stuff in this repository be automatically fixed by the next run? If so, then I recommend just doing that and scrapping this PR. This was done by hand not realizing root definitions existed elsewhere.

@lorentey
Copy link
Member

Oh, these won't be automatically regenerated -- synchronizing the filenames will just make things easier if these examples ever need to be recreated.

@SDGGiesbrecht
Copy link
Contributor Author

SDGGiesbrecht commented May 11, 2021

I addressed the feedback about existing changes.

To prevent similar issues when people try to run benchmarks on Windows, it would be a good idea to make sure the tool filters out invalid characters from the filenames it generates.

I would rather leave that for someone more familiar with the Windows than I am. It does not appear that simple: https://stackoverflow.com/a/1976050/5280938

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you 😊

@lorentey
Copy link
Member

@swift-ci test

@lorentey
Copy link
Member

I submitted #10 to fix generated filenames on Windows.

@lorentey lorentey merged commit e7daa76 into apple:main May 12, 2021
@SDGGiesbrecht SDGGiesbrecht deleted the windows branch May 12, 2021 06:55
@lorentey lorentey added this to the 0.0.3 milestone Sep 19, 2022
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.

None yet

2 participants