ipaclient/ipapython macOS compatibility fixes

[neffs]

libkrb5.so.3 is called libkrb5.dylib on macOS

[abbra]

Thanks. Do you have IPA client code working on Mac OS X?

[neffs]

It connects via RPC and user-show works. Didn't check much further.

I also created an issue: https://pagure.io/freeipa/issue/6850

EDIT: I tried it on another computer but currently python-nss fails. I'll check.

[abbra]

Ok. Let me look at it next week when I'll have time. Could you please add a short step by step instruction how you configured IPA client on Mac OS X?

[abbra]

There is a PEP8 error:
PEP-8 errors:

./ipapython/session_storage.py:11:21: E225 missing whitespace around operator

[neffs]

I added the steps here: https://pagure.io/freeipa/issue/6850

[tiran]

Please import the sys module and use use sys.platform

import sys
...
if sys.platform == 'darwin':
    LIBKRB5_FILENAME = 'libkrb5.dylib'
else:
    LIBKRB5_FILENAME = 'libkrb5.so.3'

[tiran]

@neffs thanks David. Please squash your commits into a single commit (git rebase -i @~3 and use fixup on the 2nd and 3rd commit, then git push --force).

[tiran]

Looks good to me, thanks a lot for your contribution!

I'm giving your PR a provisional ACK. Travis CI will take a while to finish (at least 30 minutes).

[tiran]

fdatasync -> fsync is ok, too.

Next time, please don't add a new patch to an approved PR. It is easier for us when you create a new PR instead.

[abbra]

Please don't set ACK yet, I'm not finished with review.

I do not want to replace fdatasync() with fsync(), this is not correct towards other platforms.
I haven't yet tested this pull request against Mac OS X, so do not set ACK yet.

[abbra]

Note that we need something similar to untitaker/python-atomicwrites@2bdd9da to behave properly on Mac OS X.

[tiran]

I wrote that fdatasync -> fsync is fine. It's my code after all.

Explanation: fdatasync is a slightly optimized version of fsync that does not flush some metadata to disk, https://linux.die.net/man/2/fdatasync

fdatasync() is similar to fsync(), but does not flush modified metadata unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled. For example, changes to st_atime or st_mtime (respectively, time of last access and time of last modification; see stat(2)) do not require flushing because they are not necessary for a subsequent data read to be handled correctly. On the other hand, a change to the file size (st_size, as made by say ftruncate(2)), would require a metadata flush.

When I write the code, I chose fdatasync because st_mtime isn't strictly required for the cache files. fdatasync is a micro-optimization that fails under macOS. Instead of making the code even more complicated, I have approved the platform agnostic fsync syscall. It doesn't hurt to flush all data to disk. The files are rarely written anyway.

[tiran]

No, we don't need to sync the directory. These are cache files. It's only important that we don't have half-written cache files on disk. A missing cache file is fine.

[abbra]

I still need to test the whole set on Mac OS X myself as we have no way to test that in CI. Thus, this PR will depend on me (or some one else from FreeIPA team) to actually test the code on Mac OS X.

[abbra]

Ok, so far I cannot build a wheel from git repo on Mac OS X as we have a number of limitations ourselves -- we need to fix our configure to allow just generating enough of ipasetup.py and make files to run python wheels code. I'll supply a separate PR for this.

[tiran]

@abbra is there any reason to delay the merge? I like to get the fixes into 4.5 for the upcoming 4.5.1 release. This commit may not be sufficient for full macOS support, but it's definitely required for macOS support. There is no harm to commit it now and fix remaining issues later.

[abbra]

Well, given that it is not officially supported yet, go ahead.

[pvoborni]

IMO this can be put to 4.5.1 (ipa-4-5 branch) but in order to do it, according to FreeIPA devel processes, it needs to be attached (have a ticket link in commit message) to opened issue in 4.5.1 milestone. Otherwise it will go only to master branch (future 4.6). If this fixes 6850, then it can be reopended for it. Otherwise please open a new issue with reasoning.

[stlaz]

This got an ACK during the last developers' meeting.

[MartinBasti]

master:

• b8b28c3 Fix libkrb5 filename for macOS
• f1c6a5d Use os.fsync instead of os.fdatasync because macOS doesn't support fdatasync