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

Add macOS support to OpenZFS #12110

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

lundman
Copy link
Contributor

@lundman lundman commented May 24, 2021

Motivation and Context

Add macOS support.

Yes, one giant commit - but don't panic.

At first; the easy, independent, changes were taken out, and individually PRed against ZOL. Now we've reached a point where there is not so many of those opportunities left.

If you spot a change you would prefer to be PRed separately, please let me know.

What's in the giant commit?

  • macOS specific files, C, C++, and obj-C source files with headers.
  • automake and build environment, Makefile changes.
  • changes needed in common source files, with an attempt to minimize these.

Changes in tests/ have been left out, and will be in some future PR. Current zfs-tests pass-rate is about 50%. This is mostly due to wrong Unix tools used etc, as opposed to failure to perform ZFS tasks.

Interesting points of ... interests...

  • configure uses sed -r, changed to $GSED, but could also handle sed -E.
  • Linux squats on mount_zfs, some rename work added to macos/mount_zfs. cmd/os/linux/mount_zfs ?
  • 5 new ZFS_PROP
  • 2 new ZFS ioctl
  • 2 new zpl_attr
  • struct spa, SPA_OS_FIELDS better?
  • ZIO_OS_FIELDS
  • Separate assembler files. Should use ENTRY or similar macro, and defined list of scratch regs
  • zfs send/recv are always through unix domain pipes. (libzfs_macos_pipefd())
  • separate zpool_read_label, but I think ZOL now has 2 paths, avoiding broken lio
  • does make clean work? make install -> any macOS files on linux/freebsd?

Description

This adds support for macOS 10.9 to macOS 12 including intel and arm64.

How Has This Been Tested?

macOS zfs-tester and numerous unfortunate users.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@lundman lundman mentioned this pull request May 24, 2021
12 tasks
@lundman lundman force-pushed the macOS branch 2 times, most recently from 5e2d62d to 65a74e3 Compare May 24, 2021 07:57
@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label May 24, 2021
@lundman lundman force-pushed the macOS branch 2 times, most recently from 4277da2 to 162912a Compare May 31, 2021 11:48
@lundman lundman force-pushed the macOS branch 6 times, most recently from ef3b4c8 to e7b6e73 Compare June 8, 2021 11:26
@lundman lundman force-pushed the macOS branch 3 times, most recently from c7316f5 to ade3311 Compare June 10, 2021 01:01
@lundman lundman force-pushed the macOS branch 12 times, most recently from 84848e9 to 96f9ab0 Compare June 23, 2021 00:01
@lundman
Copy link
Contributor Author

lundman commented Jul 19, 2023

This PR has been updated up to Sonoma (Apple clang version 15.0.0 (clang-1500.0.34.3)), x86_64 and arm64.
All warnings removed, which unfortunately meant fiddling inside shared sources some more.
If it is preferred any specific change to be pulled out in separate PR, that can be done.
arm64 can be unified instead of #ifdef when they are no longer updated.

@andrewc12
Copy link
Contributor

andrewc12 commented Oct 20, 2023

@behlendorf @lundman @mcmilk I remember that we were waiting until the next release to look at this. Since 2.2 has released is there a chance of getting this reviewed for merging?

I also remember someone brought up the ideas of

  • mostly reviewing the shared code, and
  • initially dev work not blocking on macos problems (with lundman fixing things)

@mcmilk
Copy link
Contributor

mcmilk commented Oct 22, 2023

It would be nice when this PR gets into current master. I would help afterwards with the unified asm_linkage.h ;-)

Copy link
Contributor

@mcmilk mcmilk left a comment

Choose a reason for hiding this comment

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

We need to split this big commit into smaller parts.

At least ito these 4 commits:
part1: module/os/macos/* + include/os/macos/*
part2: module/*
part3: cmd/* + tests/*
part4: config/, scripts/, contrib/* and all other things
-> If you want, I can also create an example of that.

It shouldn't interfare the current CI, so that testing other PRs is working...
Maybe @behlendorf has also some ideas what needs to be done before merging.

@lundman
Copy link
Contributor Author

lundman commented Oct 23, 2023

Couple of bonus ones in there with 5 and 6. Let me know if you want them squashed into 1-4 somewhere.

@andrewc12
Copy link
Contributor

andrewc12 commented Oct 24, 2023

I've broken out the commits into separate pull requests so I can run the tests against them
#15440
#15441
#15442
#15443
#15444
#15445

2-6
#15446

@andrewc12
Copy link
Contributor

andrewc12 commented Oct 24, 2023

@lundman I tried compiling everything without the macos code (2-6)
It complained about

automake: error: cannot open < module/os/macos/Makefile.am: No such file or directory

so if this PR ends up getting split into multiple PRs that technically counts as shared code

@lundman
Copy link
Contributor Author

lundman commented Oct 24, 2023

Yep, all Makefiles are now read in all at once - something upstream did a couple of years back, and some of those Makefiles do various things, depending on .in files etc. The Makefile changes have to be last and in one go.

part1: module/os/macos/* + include/os/macos/*

This does then inlcude lib/*macos* as well,
using grep so there are outliers, like manpage.

Signed-off-by: Jorgen Lundman <[email protected]>
part2: module/*

Signed-off-by: Jorgen Lundman <[email protected]>
part3: cmd/* + tests/*

Signed-off-by: Jorgen Lundman <[email protected]>
part4: config/, scripts/, contrib/*

Signed-off-by: Jorgen Lundman <[email protected]>
Bonus part5, the non-macos changes to
include
lib
and the top level Makefiles

Signed-off-by: Jorgen Lundman <[email protected]>
Signed-off-by: Jorgen Lundman <[email protected]>
./configure CPPFLAGS="-I/usr/local/opt/gettext/include -I/usr/local/opt/openssl/include" LDFLAGS="-L/usr/local/opt/gettext/lib/ -L/usr/local/opt/openssl/lib"
- name: build
run: |
make -j 2
Copy link

@yurikoles yurikoles Nov 5, 2023

Choose a reason for hiding this comment

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

The default GitHub-hosted macOS runner has 3 vCPUs, but you are limiting make to only 2 jobs.

Copy link
Contributor

@andrewc12 andrewc12 Nov 6, 2023

Choose a reason for hiding this comment

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

This then?

make -j`nproc`

Or

make -j$((`nproc`+1))

Copy link

@yurikoles yurikoles Nov 6, 2023

Choose a reason for hiding this comment

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

nproc is part of GNU Core Utilities, but macOS ships with CLI tools from BSD. A native macOS analogue is sysctl -n hw.ncpu, there is also a hw.physicalcpu and a hw.logicalcpu for hyper-threading.

Copy link
Contributor

@andrewc12 andrewc12 Nov 13, 2023

Choose a reason for hiding this comment

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

make -j `sysctl -n hw.ncpu`
make -j $((`sysctl -n hw.ncpu`+1))

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@andrewc12
Copy link
Contributor

#15784

Ultimately, I think you can proceed with Linux/FreeBSD changes. We've pretty much given up on merging macOS, and will plan to arrange our repo and workflow to be permanently down-stream, and have own copies of files for easier merging.

hey @lundman what the, why are you finally giving up?

@andrewc12
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants