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

Make apple frameworks optional #1909

Closed

Conversation

matthewbauer
Copy link

Previously, you had to have the apple sdk frameworks downloaded to
build on “Darwin”. There are certain cases where this is not desired,
so an Autoconf conditional is added to check for their availability.
When they are not available, proctitle & fsevents are unavailable.

These frameworks are proprietary- owned and developed by Apple Inc.
They have never been released publically so we should not make
everyone use it in a core library like libuv.

Also could not get hrtime test to work on my macOS - so had to disable it.

matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Jul 4, 2018
It’s not a good idea to rely on apple’s sdk for such a core library in
Nixpkgs. I have made a patch to libuv to make these frameworks
optional. There is also a pull request here:

libuv/libuv#1909
Previously, you had to have the apple sdk frameworks downloaded to
build on “Darwin”. There are certain cases where this is not desired,
so an Autoconf conditional is added to check for their availability.
When they are not available, proctitle & fsevents are unavailable.

These frameworks are proprietary- owned and developed by Apple Inc.
They have never been released publically so we should not make
everyone use it in a core library like libuv.
@saghul
Copy link
Member

saghul commented Jul 4, 2018

What kind of environment doesn't have those? PureDarwin?

@matthewbauer
Copy link
Author

This is for Nixpkgs which kind of acts similarly to PureDarwin. We do have access to these SDKs but since they are unfree we would like to limit their usage as much as possible especially for nonessential features.

@santigimeno
Copy link
Member

What error are you getting with the hrtime test?

#include <TargetConditionals.h>

#if !TARGET_OS_IPHONE
#if HAVE_APPLICATIONSERVICES_APPLICATIONSERVICES_H
Copy link
Member

Choose a reason for hiding this comment

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

this won't work on a GYP / CMake build I think.

@saghul
Copy link
Member

saghul commented Jul 5, 2018

I see. I'm kinda -1 here. Following that logic we'd also make IOCP optional on Windows, because it's also non-free.

@matthewbauer
Copy link
Author

Ok - I hadn't realized there was another build system. That could make things trickier. It's definitely okay to not merge - this is just what we will be using & it's a fairly straightforward patch. Note that without something like this, the fsevents tests will still fail on iOS.

Defines these on OS=mac:

- HAVE_CORESERVICES_CORESERVICES_H
- HAVE_APPLICATIONSERVICES_APPLICATIONSERVICES_H
@matthewbauer
Copy link
Author

Ok- added support for .gyp & CMake. FWIW it looks like the "HAVE_SYS_AHAFS_EVPRODS_H" check will also be broken in CMake.

It's a fairly unusual choice for a project to support 3 build systems. I certainly hope you don't plan to move away from autoconf. This would make things tricky for us because CMake depends on libuv to build itself so it's not clear how we would build libuv in the first place without autoconf.

@matthewbauer
Copy link
Author

Here is the output for hrtime:

not ok 98 - hrtime
# exit code 6
# Output from process `hrtime`:
# Assertion failed in test/test-hrtime.c on line 50: diff < (uint64_t) 80 * NANOSEC / MILLISEC

@dtzWill
Copy link

dtzWill commented Nov 15, 2018

FWIW this no longer applies cleanly (as github informs us), which I mention because I ran into this updating our (=Nixpkgs) version to the new 1.24.0 release where the patch no longer applies. Not a blocker for me but pinging this PR so at least you know :).

@matthewbauer
Copy link
Author

Ok. It is probably best to drop this patch and go back to the old way of providing the frameworks. I was hoping this would get merged so we wouldn’t have to deal with these conflicts. Now that it looks like this won’t be merged it is probably for the best to drop the patch.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2018

I'll go ahead and close this, as it seems to have stalled out. Thanks for the PR anyway!

@cjihrig cjihrig closed this Dec 14, 2018
@lo1tuma lo1tuma mentioned this pull request Jan 8, 2019
10 tasks
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

5 participants