Skip to content

Conversation

@ratchetfreak
Copy link

only windows is implemented yet

I thought that linux would let you do chdir before execvp but that means relative paths are no long relative to the current original directory.

Comment on lines +1195 to +1198
if (pwd) {
NOB_TODO("changing working directory not yet supported on linux")
}

Copy link

Choose a reason for hiding this comment

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

I tested chdir on linux and it worked correctly.
I think you made a mistake in build_and_run_test that made it appear as if it did not work correctly.

Suggested change
if (pwd) {
NOB_TODO("changing working directory not yet supported on linux")
}
if (pwd) {
if (chdir(pwd) < 0){
nob_log(NOB_ERROR, "Could not change directory for child process: %s", strerror(errno));
exit(1);
}
}

Comment on lines +44 to +45
cmd_append(cmd, temp_sprintf("%s%s%s", BUILD_FOLDER, TESTS_FOLDER, test_name));
if (!cmd_run(cmd, .pwd = test_cwd_path)) return false;
Copy link

Choose a reason for hiding this comment

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

I think there is a mistake here.
Setting pwd to be test_cwd_path and then executing test_cwd_path/test_name doesn't make sense.
When pwd is test_cwd_path the relative path to the test would be ../test_name.

Suggested change
cmd_append(cmd, temp_sprintf("%s%s%s", BUILD_FOLDER, TESTS_FOLDER, test_name));
if (!cmd_run(cmd, .pwd = test_cwd_path)) return false;
cmd_append(cmd, temp_sprintf("../%s", test_name));
if (!cmd_run(cmd, .pwd = test_cwd_path)) return false;

Copy link
Author

Choose a reason for hiding this comment

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

the code I wrote worked for windows,

CreateProcess expects the relative program path to be relative to the current working directory, not the passed in working directory. At no point does CreateProcessA search the target working directory for the application binary.

chdir in the child process makes exec not search the parent working directory. This makes the behavior between the two platforms incompatible for relative binary names.

I defaulted to windows' behavior because that's what I use.

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