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() is not capable of finding non-.exe programs on Windows #20314

Closed
alexrp opened this issue Jun 16, 2024 · 6 comments
Closed
Labels
zig build system std.Build, the build runner, `zig build` subcommand, package management
Milestone

Comments

@alexrp
Copy link
Contributor

alexrp commented Jun 16, 2024

Since the function unconditionally appends b.graph.host.result.exeFileExt() to each element in names when testing paths, it appears to just be impossible to locate e.g. npm.cmd on Windows.

My first thought here is that it should just be changed to try both with and without exeFileExt(). That way, I can pass .{"npm", "npm.cmd"} and it should just work on Windows/Unix. I can open a PR for that if it's considered an acceptable solution.

@Vexu Vexu added the zig build system std.Build, the build runner, `zig build` subcommand, package management label Jun 16, 2024
@Vexu Vexu added this to the 0.15.0 milestone Jun 16, 2024
alexrp added a commit to alexrp/zig that referenced this issue 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.

Closes ziglang#20314.
@rohlem
Copy link
Contributor

rohlem commented Jun 19, 2024

Looking at this again, we already have logic for determining whether a file has an executable suffix in std.process.Child.windowsCreateProcessSupportsExtension,
which is integrated when spawning a child process via spawn -> spawnWindows -> std.process.Child.windowsCreateProcessPathExt.
It seems like this supports further extensions (.bat, .cmd, .com, .exe) and fundamentally doesn't settle on the extended version until it's tried the non-extended one.

We might want to extract that logic and re-use it in the build system instead of doing something subtly different,
though this can be done in a follow-up PR/change to #20337 .
@squeek502 , seems like the extension logic in std.process.Child was written by you, if you want to weigh in on this.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 19, 2024

Unifying this sounds reasonable. Do we have a good place to put that logic?

@squeek502
Copy link
Collaborator

squeek502 commented Jun 19, 2024

We might want to extract that logic and re-use it in the build system

This was my initial thought as well. Note, though, that most of the complexity of that code in std.process.Child is around making it fast, which findProgram might not need to care about as much. The basic idea is pretty simple:

  • for each search path:
    • try unsuffixed command name
    • iterate PATHEXT environment variable, split by ; (can filter out any we don't support spawning via std.process.Child, that's what windowsCreateProcessSupportsExtension is for)
      • append the current extension from PATHEXT to the command name and try that

EDIT: To clarify, I think it might make sense to just reimplement a simpler version of the search path logic rather than try to make the std.process.Child implementation re-usable.

@alexrp
Copy link
Contributor Author

alexrp commented Jun 19, 2024

Looking at the std.process.Child code, it seems to only support a small subset of the default values of PATHEXT. Perhaps it would make more sense to just specifically look for the supported extensions, and add a comment to keep it in sync with std.Build.findProgram()?

@alexrp
Copy link
Contributor Author

alexrp commented Jun 19, 2024

Or, actually, I guess we would still want to respect the values in PATHEXT (e.g. if the user has removed .bat from there, we should respect that). So iterate PATHEXT, but duplicate a simplified version of the check for the supported subset of extensions?

@squeek502
Copy link
Collaborator

squeek502 commented Jun 19, 2024

Yeah, it should be possible to simplify filtering PATHEXT since it doesn't have to deal with WTF-16, so it can just use some if/elses with std.ascii.eqlIgnoreCase

(or std.StaticStringMapWithEql(void, std.static_string_map.eqlAsciiIgnoreCase), but StaticStringMap won't actually provide much/any benefit because it's only faster when the keys are of different lengths).

alexrp added a commit to alexrp/zig that referenced this issue Jun 19, 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.

Closes ziglang#20314.
alexrp added a commit to alexrp/zig that referenced this issue Jun 19, 2024
…extensions.

This roughly mirrors the logic in std.process.Child.

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 added a commit to alexrp/zig that referenced this issue Jun 19, 2024
…extensions.

This roughly mirrors the logic in std.process.Child.

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.
@andrewrk andrewrk modified the milestones: 0.15.0, 0.14.0 Jun 20, 2024
alexrp added a commit to alexrp/zig that referenced this issue Jun 21, 2024
…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 added a commit to alexrp/zig that referenced this issue Jun 21, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

No branches or pull requests

5 participants