Skip to content

fix: make sure the engine executable exists#913

Merged
Disservin merged 3 commits intoDisservin:masterfrom
leofracca:check-if-exe-exists
Sep 17, 2025
Merged

fix: make sure the engine executable exists#913
Disservin merged 3 commits intoDisservin:masterfrom
leofracca:check-if-exe-exists

Conversation

@leofracca
Copy link
Contributor

Exit right away if the engine isn’t found.
This makes the error message clearer by saying the executable doesn’t exist, rather than showing the previous generic one: "Fatal; <engine_name> engine startup failure: Couldn't start engine process."

@Disservin
Copy link
Owner

This has multiple flaws
a) the filesystem usage is currently not guarded
b) this assume cmd is an exact path which
might not be the case when used together with dir
c) we had this in the past but removed it because we allowed engines to be started from PATH which means filesystem checks will fail unless the path is expanded which I don't want to do

@leofracca
Copy link
Contributor Author

I think all three issues you mentioned can be addressed:
a) guard the std::filesystem logic
b) check if dir is defined, and concatenate with cmd before checking (maybe with an helper function and call it at the end of parseEngine and parseEach or in sanitize as it was done before #869?)
c) only run the check if we have an absolute path or dir is set

Would this be okay?

@Disservin
Copy link
Owner

yeah so for
a) you'd have to use NO_STD_FILESYSTEM, which currently also used in other places
b) that logic should go in sanitize
c) we can check if it is an absolute path, im not sure how you do the check when dir is set since you might have to check the path..

@leofracca
Copy link
Contributor Author

Pushed a new commit with the following fixes:
a) guards for filesystem
b) added dir handling and move the logic to sanitize
c) check if using absolute path

@Disservin Disservin merged commit 9aad63e into Disservin:master Sep 17, 2025
18 checks passed
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