Skip to content

Restrict overmatching MACH ifdef to only trigger on OSX and Mach#386

Merged
clalancette merged 1 commit intoros2:rollingfrom
Zopolis4:rolling
Sep 23, 2022
Merged

Restrict overmatching MACH ifdef to only trigger on OSX and Mach#386
clalancette merged 1 commit intoros2:rollingfrom
Zopolis4:rolling

Conversation

@Zopolis4
Copy link
Copy Markdown
Contributor

As said on line 43, this check is only intended for OSX.

 // Assume clock_get_time is available on OS X.

Hurd uses a different version of Mach that does not have mach/clock.h.
https://www.gnu.org/software/hurd/hurd/porting/guidelines.html

#ifdef __MACH__
Some applications put Apple Darwin-specific code inside #ifdef __MACH__ guards. Such guard is clearly not enough, since not only Apple uses Mach as a kernel. This should be replaced by #if defined(__MACH__) && defined(__APPLE__)

Signed-off-by: Maximilian Downey Twiss creatorsmithmdt@gmail.com

Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One fix inline, then this will be ready for CI.

Comment thread src/time_unix.c Outdated
@Zopolis4 Zopolis4 marked this pull request as draft September 21, 2022 22:40
Signed-off-by: Zopolis4 <creatorsmithmdt@gmail.com>
@Zopolis4 Zopolis4 marked this pull request as ready for review September 21, 2022 22:43
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Sep 22, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Zopolis4
Copy link
Copy Markdown
Contributor Author

This doesn't touch windows at all, so not sure what's going on there.

@clalancette
Copy link
Copy Markdown
Contributor

This doesn't touch windows at all, so not sure what's going on there.

We've been having some instability with the Windows workers, it looks like we ran into it. I've kicked off a new job.

@clalancette
Copy link
Copy Markdown
Contributor

The single failing test on Windows has nothing to do with this, so I'll go ahead and merge it.

@clalancette clalancette merged commit e46f124 into ros2:rolling Sep 23, 2022
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