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

C#: Replace libnethost dependency to find hostfxr #65438

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

neikeq
Copy link
Contributor

@neikeq neikeq commented Sep 6, 2022

We want to replace libnethost as it gives us issues with some compilers.
Our implementation tries to mimic libnethost's hostfxr_resolver search logic. We try to use the same function names for easier comparing in case we need to update this in the future.

@neikeq neikeq added this to the 4.0 milestone Sep 6, 2022
@neikeq neikeq requested a review from a team September 6, 2022 20:37
@neikeq neikeq requested a review from a team as a code owner September 6, 2022 20:37
@akien-mga
Copy link
Member

Gave this a try on the official buildsystem, it seems to fail building for Linux at least with our custom Linux toolchain: https://downloads.tuxfamily.org/godotengine/toolchains/

In file included from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/sysroot/usr/include/sys/types.h:222,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/sysroot/usr/include/stdlib.h:314,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/cstdlib:75,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/ext/string_conversions.h:41,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/bits/basic_string.h:6535,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/string:55,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/stdexcept:39,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/system_error:41,
                 from /root/x86_64-godot-linux-gnu_sdk-buildroot/x86_64-godot-linux-gnu/include/c++/10.2.0/mutex:42,
                 from ./core/os/mutex.h:39,
                 from ./core/os/thread_safe.h:34,
                 from ./core/object/message_queue.h:35,
                 from ./core/object/object.h:35,
                 from ./core/variant/binder_common.h:35,
                 from ./core/object/method_bind.h:34,
                 from ./core/object/class_db.h:34,
                 from ./core/object/ref_counted.h:34,
                 from ./modules/regex/regex.h:34,
                 from modules/mono/editor/semver.h:35,
                 from modules/mono/editor/semver.cpp:31:
modules/mono/editor/semver.h: In constructor 'godotsharp::SemVer::SemVer(int, int, int, const String&, const String&)':
modules/mono/editor/semver.h:80:4: error: class 'godotsharp::SemVer' does not have any field named 'gnu_dev_major'
   80 |    major(p_major),
      |    ^~~~~
modules/mono/editor/semver.h:81:4: error: class 'godotsharp::SemVer' does not have any field named 'gnu_dev_minor'
   81 |    minor(p_minor),
      |    ^~~~~

@neikeq
Copy link
Contributor Author

neikeq commented Sep 7, 2022

That seems to be some weird glibc issue. One of the included headers defines major and minor macros that expand to that.

@akien-mga
Copy link
Member

akien-mga commented Sep 7, 2022

We currently use glibc 2.19 for the official toolchain to maximize compatibility: https://github.com/godotengine/buildroot/#godot-buildroot

Even by old glibc standards it's pretty old so we'll likely bump it to something a couple of years newer eventually, but that's pending on doing a general update pass of the buildroot config.

major is indeed defined here:

$ rg "define major"
x86_64-godot-linux-gnu/sysroot/usr/include/sys/sysmacros.h
61:#define major(dev) gnu_dev_major (dev)

That being said on my Mageia 9 machine with glibc 2.36 I also have the same:

/usr/include/sys/sysmacros.h
60:#define major(dev) gnu_dev_major (dev)

No idea why sys/sysmacros.h ends un included here and why it defines such a common name. I guess potential workarounds are to use a different name or #undef major before using it.

@neikeq
Copy link
Contributor Author

neikeq commented Sep 7, 2022

Yes, I'll just undefine it. Found one such instance here:

// <tuple> includes <sys/sysmacros.h> through some other header
// this results in major(x) being resolved to gnu_dev_major(x)
// which is an expression in a constructor initializer list.
#if defined( major )
# undef major
#endif
#if defined( minor )
# undef minor
#endif

@neikeq neikeq force-pushed the replace-libnethost-dependency branch from 0c93d76 to b91d39b Compare September 7, 2022 12:41
We want to replace libnethost as it gives us issues with some compilers.
Our implementation tries to mimic libnethost's hostfxr_resolver search
logic. We try to use the same function names for easier comparing in
case we need to update this in the future.
@neikeq neikeq force-pushed the replace-libnethost-dependency branch from b91d39b to f784fb2 Compare September 7, 2022 14:36
@akien-mga akien-mga merged commit 6b92dbf into godotengine:master Sep 7, 2022
@akien-mga
Copy link
Member

Thanks!

@neikeq neikeq deleted the replace-libnethost-dependency branch September 7, 2022 18:01
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.

3 participants