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

std.Build.findProgram(): Try with and without the executable extension. #20337

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented Jun 18, 2024

While here, I also improved it to not misreport OOM from std.fs.realpathAlloc() as a generic failure to find the program, but instead panic like the rest of the build system does for OOM.

This is the fix I proposed in #20314.

@rohlem
Copy link
Contributor

rohlem commented Jun 18, 2024

I'm a bit surprised by the order being to first append ".exe" before trying the name verbatim.
Say I for some reason have test.exe and test.exe.exe.
If I specify test.exe in source code, it seems like this implementation will yield test.exe.exe instead of the explicitly specified name.

I realize this would technically be a breaking change to edge cases otherwise,
but it still seems like an unintuitive default to me.
Do we have an argument in favour of the current ordering?

@alexrp
Copy link
Member Author

alexrp commented Jun 18, 2024

That strikes me as enough of an edge case that I really have no strong feelings on it. I'm fine with either order.

@alexrp
Copy link
Member Author

alexrp commented Jun 19, 2024

@rohlem @squeek502 I think this should cover what we discussed in #20314.

@alexrp alexrp force-pushed the std-build-find-program-exe branch from 551e485 to c2e92de Compare June 19, 2024 15:06
lib/std/Build.zig Outdated Show resolved Hide resolved
@alexrp alexrp force-pushed the std-build-find-program-exe branch from c2e92de to ae08da9 Compare June 19, 2024 23:46
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is good enough to merge, however, I have a request that will eliminate the need for those "keep in sync" comments.

lib/std/process/Child.zig Outdated Show resolved Hide resolved
@alexrp alexrp force-pushed the std-build-find-program-exe branch from ae08da9 to 071c2e4 Compare June 21, 2024 01:53
…extensions.

I renamed std.process.Child.CreateProcessSupportedExtension to WindowsExtension
and made it public to avoid duplicating the list of extensions.

While here, I also improved it to not misreport OOM from std.fs.realpathAlloc()
as a generic failure to find the program, but instead panic like the rest of the
build system does for OOM.

Closes ziglang#20314.
@alexrp alexrp force-pushed the std-build-find-program-exe branch from 071c2e4 to b83bf58 Compare June 21, 2024 04:29
@alexrp
Copy link
Member Author

alexrp commented Jun 21, 2024

Ok, I think this should be good to go.

@andrewrk
Copy link
Member

Thanks!

@andrewrk andrewrk merged commit 9be9b8c into ziglang:master Jun 21, 2024
10 checks passed
@alexrp alexrp deleted the std-build-find-program-exe branch June 22, 2024 05:08
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