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

deps: ntlmclient: provide platform-independent htonll implementation #5614

Closed
wants to merge 1 commit into from

Conversation

pks-t
Copy link
Member

@pks-t pks-t commented Aug 24, 2020

With the introduction of ntlmclient, we've started playing whac-a-mole
with platforms to support the non-standard htonll function. Let's end
this game once and for all by providing a generic implementation that's
implemented via htonl(3P) and a endianness-check in CMake.

@pks-t
Copy link
Member Author

pks-t commented Aug 24, 2020

/cc @ethomson

With the introduction of ntlmclient, we've started playing whac-a-mole
with platforms to support the non-standard htonll function. Let's end
this game once and for all by providing a generic implementation that's
implemented via htonl(3P) and a endianness-check in CMake.
@pks-t pks-t force-pushed the pks/ntlmclicent-htonll-generic branch from 03fb6a7 to aaa4fda Compare August 24, 2020 10:40
@ryandesign
Copy link

Could this be done without requiring the endianness to be detected in the cmake script, but rather detecting it at build time in compat.h using preprocessor defines that are provided by the compiler? On some platforms like macOS it is possible to build for multiple architectures simultaneously (e.g. cc -arch i386 -arch x86_64 -arch ppc ...) in which case it is not possible to know at cmake time what the endianness or bitness will be at build time.

@ryandesign
Copy link

For example, __BIG_ENDIAN__ or __LITTLE_ENDIAN__ is defined by macOS compilers so this should work on macOS:

#ifdef __BIG_ENDIAN__
static inline uint64_t ntlm_htonll(uint64_t value)
{
	return value;
}
#else

# include <arpa/inet.h>

static inline uint64_t ntlm_htonll(uint64_t value)
{
	return ((uint64_t)htonl(value) << 32) + htonl((uint64_t)value >> 32);
}

#endif

I don't know what defines other systems use.

dbevans added a commit to macports/macports-ports that referenced this pull request Aug 25, 2020
Macro htonll() is not defined on these platforms.

Closes https://trac.macports.org/ticket/61057

Thanks to @ryandesign for pushing this upstream.
See libgit2/libgit2#5613
and libgit2/libgit2#5614.
@ethomson
Copy link
Member

ethomson commented Aug 25, 2020

On some platforms like macOS it is possible to build for multiple architectures simultaneously (e.g. cc -arch i386 -arch x86_64 -arch ppc ...) in which case it is not possible to know at cmake time what the endianness or bitness will be at build time.

This isn't something that we support out-of-the-box already. Our cmake recipes determine the bitness of the machine (see GIT_ARCH_64), so you couldn't -arch i386 -arch x86_64. Supporting this seems like a reasonable goal but I don't think that we should block this on that. We'd probably just want to do the quadfat magic ourselves with lipo 🤷‍♂️ .

@ethomson
Copy link
Member

Although having said that, we do need to let people who build without CMake deal with this properly.

We have a pattern for the libgit2 defines (like GIT_ARCH_64) in features.h, but we don't have any pattern for the NTLM dependency. ☹️

@ryandesign
Copy link

so you couldn't -arch i386 -arch x86_64.

Building with -arch i386 -arch x86_64 does succeed, but it is certainly possible that what gets built that way doesn't work correctly.

Supporting this seems like a reasonable goal but I don't think that we should block this on that. We'd probably just want to do the quadfat magic ourselves with lipo

You probably don't want to add any code to do multi-architecture builds. It's fine to leave that up to the users who want to do it. For the sake of MacPorts, I'll switch our libgit2 build over to the build-multiple-times-and-lipo approach.

ryandesign added a commit to ryandesign/macports-ports that referenced this pull request Aug 26, 2020
dbevans pushed a commit to macports/macports-ports that referenced this pull request Aug 26, 2020
@ethomson
Copy link
Member

Building with -arch i386 -arch x86_64 does succeed, but it is certainly possible that what gets built that way doesn't work correctly.

Interesting. I'll have to do a little investigation here. I suspect that - assuming that you're doing the build on amd64 (which I certainly hope is true 😁) that the i386 version may have some issues. I'll have to 👀

Copy link
Member

@ethomson ethomson left a comment

Choose a reason for hiding this comment

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

We'll need to add this to config.h.in to support non-CMake users.

@ethomson
Copy link
Member

Fixed with #5658 which won't require any changes to config.h.in or require non-CMake users to make any changes.

@ethomson ethomson closed this Oct 14, 2020
@ethomson ethomson deleted the pks/ntlmclicent-htonll-generic branch September 5, 2021 12:50
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.

None yet

3 participants