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

Jump to bottom

Work around compilation failure with old Apple SDKs #6602

Merged
merged 1 commit into from Feb 15, 2020
Merged

Work around compilation failure with old Apple SDKs #6602

merged 1 commit into from Feb 15, 2020

Conversation

lilyball
Copy link
Contributor

When building fish-shell with the macOS 10.12 SDK, <sys/proc.h> does not include <sys/time.h> but references struct itimerval. This causes a compilation failure if we don't import <sys/time.h> ourselves.

This was previously masked by an import of <sys/sysctl.h>, which was removed in fc0c39b.

This fixes a build failure when building Fish using Nix, as Nix is currently on the macOS 10.12 SDK.

@lilyball
Work around compilation failure with old Apple SDKs
10385d4 
When building fish-shell with the macOS 10.12 SDK, <sys/proc.h> does not
include <sys/time.h> but references `struct itimerval`. This causes a
compilation failure if we don't import <sys/time.h> ourselves.

This was previously masked by an import of <sys/sysctl.h>, which was
removed in fc0c39b.
@lilyball lilyball mentioned this pull request Feb 14, 2020
Merged
10 tasks
zanchey
zanchey reviewed Feb 14, 2020
View reviewed changes
src/expand.cpp
@@ -17,6 +17,7 @@
#include <procfs.h>
#endif
#if __APPLE__
#include <sys/time.h> // Required to build with old SDK versions
Copy link
Member

Choose a reason for hiding this comment

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

possibly make this an IWYU pragma?

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'm not set up to run IWYU so I don't know if it would even flag this. We do use something from this file, it's just that in newer SDKs it's redundant because <sys/proc.h> includes it.

@floam
Copy link
Member

floam commented Feb 14, 2020

Could hide it behind a check specifically for the 10.12 SDK or older before including it. Not really going to make a difference though.

@lilyball
Copy link
Contributor Author

@floam I don't know which SDK added the #include <sys/time.h> to <sys/proc.h> so I don't feel comfortable making it conditional.

@zanchey zanchey added this to the fish 3.1.1 milestone Feb 14, 2020
@zanchey
Copy link
Member

zanchey commented Feb 14, 2020

SGTM! I think we're definitely going to have to do a 3.1.1, so I'll add this to the milestone and cherry-pick it when I get a chance.

@faho faho merged commit 47aeaa1 into master Feb 15, 2020
@faho faho deleted the old_sdk_compilation branch February 15, 2020 09:59
@faho
Copy link
Member

faho commented Feb 15, 2020

I've just merged this now, as-is, because it fixes what it purports properly. I think we'd have to re-check a bunch of includes anyway if we reran IWYU (I think the last time was in 2017?).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zanchey zanchey zanchey left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
fish 3.1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lilyball @floam @zanchey @faho