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

Fix windows chdir behavior #365

Merged
merged 6 commits into from
Jun 13, 2024
Merged

Fix windows chdir behavior #365

merged 6 commits into from
Jun 13, 2024

Conversation

dethrace-labs
Copy link
Owner

@dethrace-labs dethrace-labs commented Jun 13, 2024

argv[0] was expected to include the full path to the dethrace executable. On windows, this is not true, and only includes the filename.

As a fix, we should only try to chdir if root_dir is not empty or a single character.

result = chdir(root_dir);
if (result != 0) {
LOG_PANIC("Failed to chdir. Error is %s", strerror(errno));
if (root_dir != NULL && strlen(root_dir) > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

On discord, you said "." worked.
What about only ignoring an empty root_dir?

Suggested change
if (root_dir != NULL && strlen(root_dir) > 1) {
if (root_dir != NULL && root_dir[0] != '\0') {

Copy link
Owner Author

Choose a reason for hiding this comment

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

My thinking was if the value is ., we don't need to call chdir since it is a no-op. I suppose I could do a strcmp(root_dir, '.') to make that more obvious

Copy link
Collaborator

@madebr madebr Jun 13, 2024

Choose a reason for hiding this comment

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

It's a no-op, but I don't think we should care. chdir's implementation should be able to handle it.
The current change would disallow one-letter directories.
An unlikely directory structure, but possible.

@@ -220,6 +220,10 @@ void GLRenderer_Init(int pRender_width, int pRender_height) {
LOG_INFO("OpenGL version string: %s", glGetString(GL_VERSION));
LOG_INFO("OpenGL shading language version string: %s", glGetString(GL_SHADING_LANGUAGE_VERSION));

if (glGetString(GL_SHADING_LANGUAGE_VERSION) == NULL) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

this resolves a null pointer error inside glCreateProgram later on inside my windows VM which doesn't expose gl shaders

Copy link
Collaborator

Choose a reason for hiding this comment

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

What OpenGL version does the vm expose?

But you're probably correct to test capabilities instead of requiring a minimum OpenGL version.

src/harness/harness.c Outdated Show resolved Hide resolved
src/harness/harness.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@madebr madebr left a comment

Choose a reason for hiding this comment

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

With my last comment, LGTM!

Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>
@dethrace-labs
Copy link
Owner Author

thanks for the spotting! I must be tired today :)

@dethrace-labs dethrace-labs merged commit 3870bf8 into main Jun 13, 2024
4 checks passed
@dethrace-labs dethrace-labs deleted the fix_windows_chdir branch June 13, 2024 22:31
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.

3 participants