This is an archive of the discontinued LLVM Phabricator instance.

Differential D158584

[libc++] Fix GNU/Hurd build
ClosedPublic

Authored by sthibaul on Aug 23 2023, 12:32 AM.

Details

Reviewers
ldionne
phosek
Mordante
Group Reviewers
Restricted Project
Commits
rG1cfcc36812ff: [libc++] Fix GNU/Hurd build
Summary

GNU/Hurd does have clock_gettime, it just doesn't define _POSIX_TIMERS because its support for timers is not complete.

Diff Detail

Event Timeline

sthibaul created this revision.Aug 23 2023, 12:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 12:32 AM
Herald added a subscriber: krytarowski. · View Herald Transcript
sthibaul requested review of this revision.Aug 23 2023, 12:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 12:32 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
Harbormaster completed remote builds in B254265: Diff 552611.Aug 23 2023, 2:15 AM
Mordante added a subscriber: Mordante.Aug 23 2023, 9:13 AM

No real objections, but I like to use a better macro if possible.

Another question in general libc++ likes to have a pre-commit CI (https://buildkite.com/llvm-project) for all platforms we support. Would it be possible to provide a CI runner for GNU/Hurd?

libcxx/src/chrono.cpp
34

Based on https://github.com/cpredef/predef/blob/master/OperatingSystems.md this macro works too. When that information is correct I prefer this macro; it easier to understand this is for the GNU/Hurd platform and it's not a typo of __GNUC__.

Having the full name makes it easier to understand which OS this is. I know it from the context of the patch. but __GNU__ does not ring a bell for me, and I know GNU/Hurd is an OS.

sthibaul updated this revision to Diff 552898.Aug 23 2023, 2:58 PM

Use gnu_hurd instead of GNU

sthibaul marked an inline comment as done.Aug 23 2023, 3:02 PM

Another question in general libc++ likes to have a pre-commit CI (https://buildkite.com/llvm-project) for all platforms we support. Would it be possible to provide a CI runner for GNU/Hurd?

If we can, sure. What technology is used for that? Is that buildbot https://lab.llvm.org/buildbot/#/builders ? How much load would that give?

libcxx/src/chrono.cpp
34

Right, why not.

Harbormaster completed remote builds in B254476: Diff 552898.Aug 23 2023, 10:07 PM
Mordante added a comment.Aug 25 2023, 9:36 AM
In D158584#4611806, @sthibaul wrote:

Another question in general libc++ likes to have a pre-commit CI (https://buildkite.com/llvm-project) for all platforms we support. Would it be possible to provide a CI runner for GNU/Hurd?

If we can, sure. What technology is used for that? Is that buildbot https://lab.llvm.org/buildbot/#/builders ? How much load would that give?

At the moment we use Buildkite @ldionne can give you a BuildKite Agent token. The advantage is we can officially support GNU/Hurd. In general we prefer to validate patches in the CI, especially when larger changes are needed.

Mordante accepted this revision.Aug 25 2023, 9:37 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 25 2023, 9:37 AM
sthibaul marked an inline comment as done.Aug 25 2023, 9:50 AM

(I don't have commit access)

sthibaul added a comment.Aug 25 2023, 9:55 AM

At the moment we use Buildkite

Is this free software? The installation instructions are not talking about source code, only pre-built binaries, which are of course not available for GNU/Hurd.

Mordante added a comment.Aug 25 2023, 10:17 AM
In D158584#4617472, @sthibaul wrote:

(I don't have commit access)

Can you provide your name and e-mail address, then I'll commit it on your behalf.

In D158584#4617495, @sthibaul wrote:

At the moment we use Buildkite

Is this free software? The installation instructions are not talking about source code, only pre-built binaries, which are of course not available for GNU/Hurd.

Good question. I'm not sure. In the future we plan to move to github actions, maybe that's easier to use on GNU/Hurd.

sthibaul added a comment.Aug 25 2023, 10:46 AM

Can you provide your name and e-mail address, then I'll commit it on your behalf

Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!

Closed by commit rG1cfcc36812ff: [libc++] Fix GNU/Hurd build (authored by sthibaul, committed by Mordante). · Explain WhyAug 25 2023, 10:55 AM
This revision was automatically updated to reflect the committed changes.
Mordante added a commit: rG1cfcc36812ff: [libc++] Fix GNU/Hurd build.
sthibaul added a comment.Aug 25 2023, 11:18 AM

Could you also backport this to the 17 branch? Thanks

Mordante added a comment.Aug 26 2023, 3:40 AM
In D158584#4617801, @sthibaul wrote:

Could you also backport this to the 17 branch? Thanks

I've prepared a PR for the release manager https://github.com/llvm/llvm-project-release-prs/pull/650

Revision Contents

PathSize
libcxx/
src/
2 lines
filesystem/
2 lines

Diff 552898

libcxx/src/chrono.cpp

Show All 25 Lines
#if __has_include(<unistd.h>) #if __has_include(<unistd.h>)
# include <unistd.h> // _POSIX_TIMERS # include <unistd.h> // _POSIX_TIMERS
#endif #endif
#if __has_include(<sys/time.h>) #if __has_include(<sys/time.h>)
# include <sys/time.h> // for gettimeofday and timeval # include <sys/time.h> // for gettimeofday and timeval
#endif #endif
#if defined(__APPLE__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0) #if defined(__APPLE__) || defined (__gnu_hurd__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
MordanteUnsubmitted
Done
ReplyInline Actions
#endif
- #if defined(__APPLE__) || defined (__GNU__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
+ #if defined(__APPLE__) || defined (__gnu_hurd__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
# define _LIBCPP_HAS_CLOCK_GETTIME

Based on https://github.com/cpredef/predef/blob/master/OperatingSystems.md this macro works too. When that information is correct I prefer this macro; it easier to understand this is for the GNU/Hurd platform and it's not a typo of __GNUC__.

Having the full name makes it easier to understand which OS this is. I know it from the context of the patch. but __GNU__ does not ring a bell for me, and I know GNU/Hurd is an OS.

Mordante: Based on https://github.com/cpredef/predef/blob/master/OperatingSystems.md this macro works too.
sthibaulAuthorUnsubmitted
Done
ReplyInline Actions

Right, why not.

sthibaul: Right, why not.
# define _LIBCPP_HAS_CLOCK_GETTIME # define _LIBCPP_HAS_CLOCK_GETTIME
#endif #endif
#if defined(_LIBCPP_WIN32API) #if defined(_LIBCPP_WIN32API)
# define WIN32_LEAN_AND_MEAN # define WIN32_LEAN_AND_MEAN
# define VC_EXTRA_LEAN # define VC_EXTRA_LEAN
# include <windows.h> # include <windows.h>
# if _WIN32_WINNT >= _WIN32_WINNT_WIN8 # if _WIN32_WINNT >= _WIN32_WINNT_WIN8
▲ Show 20 Lines • Show All 212 Lines • Show Last 20 Lines

libcxx/src/filesystem/filesystem_clock.cpp

Show All 23 Lines
#if __has_include(<unistd.h>) #if __has_include(<unistd.h>)
# include <unistd.h> // _POSIX_TIMERS # include <unistd.h> // _POSIX_TIMERS
#endif #endif
#if __has_include(<sys/time.h>) #if __has_include(<sys/time.h>)
# include <sys/time.h> // for gettimeofday and timeval # include <sys/time.h> // for gettimeofday and timeval
#endif #endif
#if defined(__APPLE__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0) #if defined(__APPLE__) || defined (__gnu_hurd__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
# define _LIBCPP_HAS_CLOCK_GETTIME # define _LIBCPP_HAS_CLOCK_GETTIME
#endif #endif
_LIBCPP_BEGIN_NAMESPACE_FILESYSTEM _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM
const bool _FilesystemClock::is_steady; const bool _FilesystemClock::is_steady;
_FilesystemClock::time_point _FilesystemClock::now() noexcept { _FilesystemClock::time_point _FilesystemClock::now() noexcept {
Show All 24 Lines