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

pgo/env: Account for .exe extensions during path resolution #1

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

osiewicz
Copy link
Contributor

@osiewicz osiewicz commented Aug 3, 2022

Currently cargo-pgo does not work out of the box on Windows, because
llvm-tools-preview binaries have .exe extensions. This commit extends
find_pgo_env to look up llvm-profdata with .exe extension in case one
without it is not available.

I have verified manually that renaming llvm-profdata.exe to llvm-profdata manually works with current main. bolt/env probably has the same issue on Windows, but I didn't look into that. Should we tackle BOLT in this PR as well?
By the way, thanks for this library - it looks very neat!

Currently cargo-pgo does not work out of the box on Windows, because
llvm-tools-preview binaries have .exe extensions. This commit extends
find_pgo_env to look up llvm-profdata with .exe extension in case one
without it is not available.
@Kobzol
Copy link
Owner

Kobzol commented Aug 4, 2022

Hi, thanks for the PR! As you have probably noticed, I haven't really tested this crate outside of Linux, so there are probably more demons lurking here. I'm not even sure if BOLT is supported on Windows or whether it works there at all, I'll try to check it later, for now this seems like an easy fix to make PGO working on Windows. Thanks!

@Kobzol Kobzol merged commit 5ce5096 into Kobzol:main Aug 4, 2022
@Kobzol
Copy link
Owner

Kobzol commented Aug 4, 2022

I added basic Windows CI tests (#4), so hopefully in the future such things will be easier to detect (at least for PGO, BOLT is currently not tested on CI).

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