-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
python3Packages.pyobjc-core: init at 10.3.1 #336801
Conversation
e7490ab
to
c628cdb
Compare
Hi, can anyone please review? |
3879755
to
8c5df9a
Compare
# | ||
# ZZZ: Sorted out the issues with tests and put them in prePatch | ||
checkPhase = '' | ||
python3 setup.py test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is deprecated and we removed already all usages of running tests like that. Please use pytest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not manage to do that, looking into the actual code, the author of the package did some extra stuff to bootstrap unittest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use pytestCheckHook
and pytestFlagsArray = [ "PyObjCTest" ]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what I tried and it was failing with
ERROR PyObjCTest/test_voidpointer.py::TestCase - AttributeError: 'TestCase' object has no attribute 'runTest'. Did you mean:...
ERROR PyObjCTest/test_voidpointer.py::TestVoidPointer - AttributeError: 'TestVoidPointer' object has no attribute 'runTest'. Did yo...
ERROR PyObjCTest/test_weakref.py::TestCase - AttributeError: 'TestCase' object has no attribute 'runTest'. Did you mean:...
ERROR PyObjCTest/test_weakref.py::TestWeakrefs - AttributeError: 'TestWeakrefs' object has no attribute 'runTest'. Did you m...
ERROR PyObjCTest/test_weakref.py::TestObjCWeakRef - AttributeError: 'TestObjCWeakRef' object has no attribute 'runTest'. Did yo...
!!!!!!!!!!!!!!!!!! Interrupted: 438 errors during collection !!!!!!!!!!!!!!!!!!!
====================== 11 warnings, 438 errors in 20.36s =======================
unittestCheckHook
is promising, trying to make it work as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
niether unittestCheckHook is easy to make working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With unittestCheck Hook errors are different and seem to be related to how tests are set up in the library
https://github.com/ronaldoussoren/pyobjc/blob/master/pyobjc-core/setup.py#L216-L329
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR with all the comments taken into account, but for running tests with all the guidelines.
would be great to have this merged |
8c5df9a
to
807e3a4
Compare
Any more issues there? |
5931b48
to
986077d
Compare
@@ -0,0 +1,117 @@ | |||
# inpired by https://github.com/input-output-hk/lace/blob/0fd166666cc454171811b307050d0815452bea82/nix/lace-blockchain-services/internal/any-darwin.nix#L339 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# inpired by https://github.com/input-output-hk/lace/blob/0fd166666cc454171811b307050d0815452bea82/nix/lace-blockchain-services/internal/any-darwin.nix#L339 |
done | ||
# impurities: | ||
( grep -RF '/usr/bin/xcrun' || true ; ) | cut -d: -f1 | while IFS= read -r file ; do | ||
sed -r "s+/usr/bin/xcrun+$(${lib.getExe pkgs.which} xcrun)+g" -i "$file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to use which instead of pkgs.which
checkPhase = '' | ||
# TODO: This library does not follow standard testing with pytest | ||
# and implemented its own test runner bootstrapping unittest | ||
python3 setup.py test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That command is deprecated. Can we use pytest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, it just fails with internal errors, not running any tests. In the source code testing is managed by this code and has additional bootstrapping in setup.py
let | ||
appleSDK = darwin.apple_sdk_11_0; | ||
inherit (appleSDK) MacOSX-SDK objc4; | ||
darwinFrameworks = appleSDK.frameworks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reduce the shorthands?
986077d
to
c0ea0f0
Compare
It looks like the oustanding requested changes, with the exception of the |
Not yet on matrix, let me join |
This is about to become much simpler. A build of The rest of the build becomes easier. You can remove the I can take a look at the unusual test setup and see if I can fit it into the standard model. The same for replacing the substitutions with patches (it's much clearer to read then). |
I was waiting on the checks to finish before merging #354108. It’s merged now, so you should be able to use it for pyobjc. |
@ferrine could you rebase this over master, please? It's the one thing I can't do for you directly. |
Did that today and got two errors in tests, will push soon |
c0ea0f0
to
2ddb668
Compare
These new errors appeared
|
# TODO: Make patch for setup.py | ||
# ignore the manual include flag for ffi, appears that it needs a very specific ffi from sdk (needs confirmation) | ||
substituteInPlace setup.py --replace-fail '"-I/usr/include/ffi"' '#"-I/usr/include/ffi"' | ||
# make os.path.exists that can spoil objc4 return True | ||
substituteInPlace setup.py --replace-fail 'os.path.join(self.sdk_root, "usr/include/objc/runtime.h")' '"/"' | ||
|
||
# Turn off clang’s Link Time Optimization, or else we can’t recognize (and link) Objective C .o’s: | ||
sed -r 's/"-flto=[^"]+",//g' -i setup.py | ||
# Fix some test code: | ||
grep -RF '"sw_vers"' | cut -d: -f1 | while IFS= read -r file ; do | ||
sed -r "s+"sw_vers"+"/usr/bin/sw_vers"+g" -i "$file" | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the test failures, but all of this should now be unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the SDK and LTO stuff should be unnecessary. LTO was fixed with the ld64 update in #307880.
The sw_vers
stuff might still be?
# Fixes impurities in the package and fixes darwin min version | ||
# TODO: Propose a patch that fixes it in a better way | ||
# Force it to target our ‘darwinMinVersion’, it’s not recognized correctly: | ||
grep -RF -- '-DPyObjC_BUILD_RELEASE=%02d%02d' | cut -d: -f1 | while IFS= read -r file ; do | ||
sed -r '/-DPyObjC_BUILD_RELEASE=%02d%02d/{s/%02d%02d/${ | ||
lib.concatMapStrings (lib.fixedWidthString 2 "0") ( | ||
lib.splitString "." stdenv.targetPlatform.darwinMinVersion | ||
) | ||
}/;n;d;}' -i "$file" | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also likely unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
darwinMinVersion
probably shouldn’t be used even if it were. Build scripts should get the minimum version from MACOSX_DEPLOYMENT_TARGET
.
This is one of the last things to merge cachix/devenv-nixpkgs#9, how can I help? |
I believe @sarahec was working on this. I’ll ask if there’s anything I can do to help the process, since this blocks a lot of things. Edit: Though looking at the current state of the PR it seems like it should probably be good to merge with a little clean‐up and figuring out what to do with the failing tests (maybe just disable them if we’re already disabling a bunch?) |
Apologies. I spent yesterday getting an updated ID and passport and have been away from the keyboard. |
I have this working and just need to figure out how to send the changes |
Ugh. There's a bunch of patches missing. I may have to open a fresh PR and link back to this one. |
I'm happy if you help with this |
I finished the patch. Unfortunately, I had to open a new PR (#358063) It couldn't be done here in suggestions, nor could I push code to this PR. |
No problems, the progress you did is huge step forward anyway |
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.