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 FTBFS on GNU/Hurd platform #2164

Closed
wants to merge 1 commit into from
Closed

Fix FTBFS on GNU/Hurd platform #2164

wants to merge 1 commit into from

Conversation

apoleon
Copy link
Contributor

@apoleon apoleon commented Jan 20, 2015

Here we go again.

Minetest fails to build on GNU/Hurd due to a name clash with OSX/Apple,
both are defining the MACH keyword. This commit fixes the issue.

@@ -360,6 +360,9 @@ inline u32 getDeltaMs(u32 old_time_ms, u32 new_time_ms)
}
#elif defined(_WIN32)
inline void setThreadName(const char* name) {}
#elif defined(__GNU__)
//#warning "GNU/Hurd platform, pthread_setname_np not yet supported"
inline void setThreadName(const char* name) {}
Copy link
Member

Choose a reason for hiding this comment

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

This does effectively the same thing as the default. Also, either the warning should be enabled or it should be a regular comment.

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 suggest to keep the conditional because GNU/Hurd is now a distinct and
known platform and it might be possible that the behaviour will be
different from the default in the future. The comment can be omitted.

Copy link
Member

Choose a reason for hiding this comment

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

You can actually combine this with the Windows section too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GNU/Hurd is not Windows. A change in WIN32 or the default might have a
negative impact on GNU/Hurd again in the future. Hence I think it is
reasonable to make this distinction clear.

Copy link
Member

Choose a reason for hiding this comment

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

Hurd doesn't support thread names though, same as Windows, so it's functionality is the same and it should be combined to reduce duplication. Hurd obviously isn't Windows, and I wasn't implying that. If Hurd or Windows get thread name support they can easily be given their own sections.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @ShadowNinja

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please tell me exactly how you want this to be fixed since I am not the
original patch creator. You can also fix this part in any way you see fit.

Copy link
Member

Choose a reason for hiding this comment

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

you need to combine windows and hurd

@ShadowNinja ShadowNinja added @ Build CMake, build scripts, official builds, compiler and linker errors Low priority labels Feb 7, 2015
Minetest fails to build on GNU/Hurd due to a name clash with OSX/Apple,
both are defining the __MACH__ keyword. This commit fixes the issue.
@apoleon
Copy link
Contributor Author

apoleon commented Feb 8, 2015

Pull request was updated. Please check again.

@nerzhul
Copy link
Member

nerzhul commented Feb 10, 2015

Seems to be good. I agree for a merge

@Zeno-
Copy link
Contributor

Zeno- commented Feb 11, 2015

commit cfca5f9

@Zeno- Zeno- closed this Feb 11, 2015
@apoleon apoleon deleted the hurd branch February 21, 2015 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Build CMake, build scripts, official builds, compiler and linker errors Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants