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 NetBSD executable path #84469

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Fix NetBSD executable path #84469

merged 1 commit into from
Jan 4, 2024

Conversation

time-killer-games
Copy link
Contributor

@time-killer-games time-killer-games commented Nov 5, 2023

No description provided.

}
String b;
b.parse_utf8(buf);
return b;
Copy link
Contributor

Choose a reason for hiding this comment

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

The branch looks sound but I wonder whether it'd make more sense to avoid duplicating the exact same code twice with something like this:

#elif defined(__NetBSD__) || defined(__FreeBSD__)
#if defined (__NetBSD__)
	int mib[4] = { CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME };
#elif
	int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1 };
#endif // defined(__NetBSD__)
	char buf[MAXPATHLEN];
	size_t len = sizeof(buf);
	if (sysctl(mib, 4, buf, &len, nullptr, 0) != 0) {
		WARN_PRINT("Couldn't get executable path from sysctl");
		return OS::get_executable_path();
	}
	String b;
	b.parse_utf8(buf);
	return b;

It's a bit uglier as I don't think that we can indent defines according to our style guide but I suppose it's going to be always the same basic structure across BSDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was wondering how you felt about that. I reviewed former code and it appeared you guys preferred having separate copies of code for each platform, but I am more than happy to remove the redundancy. I wanted to do that initially as my personal preference, but I wasn't sure what everyone else's thought was on the matter.

Thank you for the code review. I will make that change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In regards to structure, DragonFly BSD uses the exact same code as as FreeBSD 1:1 without a single line touched. OpenBSD doesn't even have this feature implemented OS-level by contrast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert my commit I just made if everyone decides to stick with my original code change that kept the duplicate code, though, I wouldn't recommend it. I prefer what you suggested.

Copy link
Contributor Author

@time-killer-games time-killer-games Nov 6, 2023

Choose a reason for hiding this comment

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

Additionally, there is a bug with NetBSD that if argv[0] is a relative path (for example: ./a.out) it will return an absolute path by KERN_PROC_PATHNAME sysctl(), though in my experience, it keeps the "relative dots" in the string. For example, if argv[0] is ./a.out, KERN_PROC_PATHNAME will give you a path looking like /home/netbsd/./a.out for example, when we would want /home/netbsd/a.out instead. So, my proposal is to clean up after this NetBSD bug and normalize the pathname with a call to realpath. Just my thought on this. There are other cases where it keeps the relative dots iirc, I just can't recall when. realpath would fix it. FreeBSD doesn't have this bug, so that would be a strenuous call to realpath for no reason, meaning we would have to make the code even more difficult to read by adding an additional #if defined(...) to address this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, there is a bug with NetBSD that if argv[0] is a relative path

Oh, I had no idea.

If that's the case then unfortunately I think it might make more sense to copy-paste two branches, one with realpath for NetBSD and one without it for FreeBSD 😅

Sorry for the backward and forwards and thanks for your patience :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! Sure thing.

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Nov 6, 2023

I forgot to use a proper commit message for that last one. My bad.

@Riteo
Copy link
Contributor

Riteo commented Nov 6, 2023

Great!

I forgot to use a proper commit message for that last one. My bad.

Note that we don't accept PRs with multiple commits (and we have disabled squash-merging for various reasons), so you'll have to squash them yourself, where you will also be able to rename the resulting commit.

You'll be rewriting history, so don't be afraid of force-pushing!

There's a guide in the documentation explaining the process: https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

@time-killer-games
Copy link
Contributor Author

I still only know the bare basics of git commands. This will be an interesting adventure. lol

@AThousandShips
Copy link
Member

One final nitpick, please remove the remark in the commit message, no need to make such a comment

@time-killer-games
Copy link
Contributor Author

Great!

I forgot to use a proper commit message for that last one. My bad.

Note that we don't accept PRs with multiple commits (and we have disabled squash-merging for various reasons), so you'll have to squash them yourself, where you will also be able to rename the resulting commit.

You'll be rewriting history, so don't be afraid of force-pushing!

There's a guide in the documentation explaining the process: https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

I believe I did it right. Correct?

@time-killer-games
Copy link
Contributor Author

One final nitpick, please remove the remark in the commit message, no need to make such a comment

I'll look up real quick how to remove that. Thanks for the info. Yeah, I should've thought of that.

@AThousandShips
Copy link
Member

git commit --amend and update the message :)

@time-killer-games
Copy link
Contributor Author

Thanks a ton!

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Looks fine! :)

@akien-mga akien-mga added bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jan 4, 2024
@akien-mga akien-mga changed the title Fix NetBSD Executable Path Fix NetBSD executable path Jan 4, 2024
@akien-mga akien-mga merged commit c921b65 into godotengine:master Jan 4, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants