Skip to content

Change default executable name when on windows#20

Closed
Rafaeruo wants to merge 1 commit intomasterfrom
windows-exe
Closed

Change default executable name when on windows#20
Rafaeruo wants to merge 1 commit intomasterfrom
windows-exe

Conversation

@Rafaeruo
Copy link
Owner

@Rafaeruo Rafaeruo commented Dec 9, 2024

This changes the default behavior of EvaluatorManager when no pkl executable path is provided.

We used to default to the executable name pkl, which was our best guess of the name of pkl's executable in the user's PATH.

When on Windows, I think it would make more sense to add the .exe file extension to the file name: pkl.exe. Although we can't be sure that the user's PATH will contain an executable named exactly that way, it's serves as a much better guess for the platform.

@jeremyvaartjes
Copy link
Collaborator

Not sure that we need to optimise to this level but this might be a more "performant" version of the function

public static string DefaultPklExecutableName
#if TARGET_WINDOWS
    => "pkl.exe";
#else
    => "pkl";
#endif

I'm guessing this PR was triggered by the failing github actions builds. This is due to an issue with the setup pkl build step and not because of the .exe being dropped from our dotnet code. I've actually put in a fix here: pkl-community/setup-pkl#41 but the issue is that github is still pointing to v0.0.5 and not what is currently in the master branch. I might ping the lead dev of that repo to see if we can get a v0.0.6 to be put together.

Copy link
Collaborator

@jeremyvaartjes jeremyvaartjes left a comment

Choose a reason for hiding this comment

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

Happy with this code either way. Feel free to use my comment above or ignore it, I don't think it makes much difference tbh.

@Rafaeruo
Copy link
Owner Author

I'm guessing this PR was triggered by the failing github actions builds.

That is correct.

but the issue is that github is still pointing to v0.0.5 and not what is currently in the master branch

I did notice that, but I also assumed that fixing the issue would require both updating pkl-setup and making this change in the way we call pkl in our code. That assumption was incorrect, though: apparently Windows allows omitting the file extension when running programs.

image

@Rafaeruo Rafaeruo closed this Dec 10, 2024
@Rafaeruo Rafaeruo deleted the windows-exe branch March 3, 2025 00:05
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