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

Support successfully bootstrapping on OpenBSD. #1070

Merged
merged 1 commit into from
Apr 26, 2018

Conversation

lanurmi
Copy link
Contributor

@lanurmi lanurmi commented Apr 26, 2018

This PR depends on PR #1068, which should be applied first.

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

If we support FreeBSD and OpenBSD, does this guarantee most other BSD variants or should we look at supporting others?

@@ -365,7 +365,7 @@ int premake_locate_executable(lua_State* L, const char* argv0)
}
#endif

#if PLATFORM_BSD
#if PLATFORM_BSD && !defined(__OpenBSD__)
Copy link
Member

Choose a reason for hiding this comment

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

This seems not great - is there something that can be done to support OpenBSD in this block instead? What does this block do on OpenBSD? I got it working on FreeBSD by adding that sysctl call, is there something similar for OpenBSD?

Sorry to bombard you with questions! I'm keen to see what we can do to improve the BSD situation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to a comment in the latter stackoverflow post referred to in premake_locate_executable()'s comments, OpenBSD does not support other methods than using argv[0]. I'm no expert in *BSD, so I cannot be absolutely sure if this is true. However, that particular piece of code fails to compile on OpenBSD due to the undeclared identifier KERN_PROC_PATHNAME.

Copy link
Member

Choose a reason for hiding this comment

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

That's fair enough, perhaps wrapping that block instead would be better? Not required, if the none of it will work on OpenBSD it's probably pointless reducing it down to what doesn't compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only KERN_PROC_PATHNAME was left out on OpenBSD, I don't think the sysctl would do anything meanful anymore even if it would compile; if that's what you meant. Also here's a comment by Theo de Raadt about finding executable path.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that entire if block that contains the call to sysctl, but it's not really worth the effort. Thanks for the link, it's a pretty interesting discussion.

Bootstrap.mak Outdated
@@ -89,7 +89,7 @@ bsd: $(SRC)
$(CC) -o build/bootstrap/premake_bootstrap -DPREMAKE_NO_BUILTIN_SCRIPTS -DLUA_USE_POSIX -DLUA_USE_DLOPEN -I"$(LUA_DIR)" -I"$(LUASHIM_DIR)" $? -lm
./build/bootstrap/premake_bootstrap embed
./build/bootstrap/premake_bootstrap --to=build/bootstrap gmake
$(MAKE) -C build/bootstrap -j`getconf _NPROCESSORS_ONLN` config=$(CONFIG)
$(MAKE) -C build/bootstrap -j`getconf _NPROCESSORS_ONLN || getconf NPROCESSORS_ONLN` config=$(CONFIG)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to remove _NPROCESSORS_ONLN or does this depend on the BSD flavour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it seems _NPROCESSORS_ONLN doesn't work on FreeBSD either; so the underscore-less variant would be fine for both OpenBSD and FreeBSD. But as someone else added the variant with underscore in the first place, dunno if it actually works on some *BSD.

Copy link
Member

Choose a reason for hiding this comment

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

That would have been me, I just copied the Linux one and it built the project, I didn't investigate the value. If it doesn't work on your FreeBSD it likely didn't work on mine, I think we should remove the underscore version. 👍

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, when it doesn't work it is not easily noticeable with GNU Make, as it accepts -j without a numeric value too, and the build doesn't fail. I was (unintentionally) using a non-gnu make on OpenBSD, which then failed because it requires a value for -j. And yes I agree, the underscore version should be removed.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, can you remove it in this PR? We can merge this once you've done that. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. 😄

@lanurmi
Copy link
Contributor Author

lanurmi commented Apr 26, 2018

I tried compiling also on NetBSD, but encountered way more fundamental errors (such as compiler not supporting long long -- not sure if there's something badly wrong with my installation, or is it supposed to be like that). In any case, I haven't tested other BSD variants besides OpenBSD and FreeBSD, but of course supporting all of them would be preferable.

@lanurmi lanurmi force-pushed the fix-openbsd-depends-on-1068 branch from 63a83af to e71a99d Compare April 26, 2018 13:40
@samsinsane samsinsane merged commit 53c4352 into premake:master Apr 26, 2018
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