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
python37: fix darwin build #42894
python37: fix darwin build #42894
Conversation
Success on x86_64-linux (full log) Attempted: python37 Partial log (click to expand)
|
Success on aarch64-linux (full log) Attempted: python37 Partial log (click to expand)
|
Success on x86_64-darwin (full log) Attempted: python37 Partial log (click to expand)
|
|
||
# Fixes darwin build. | ||
(fetchpatch { | ||
url = https://github.com/LnL7/cpython/commit/6750bb7abfb13775e1bae3143099ead4dea68d6a.patch; |
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.
Why do you download the patch from your fork instead of including it in Nixpkgs? This makes it harder for others to change it.
It seems that this patch is better (and may be proposed for inclusion upstream):
#ifdef HAVE_LIBUTIL_H
#include <libutil.h>
-#else
+#endif /* HAVE_LIBUTIL_H */
#ifdef HAVE_UTIL_H
#include <util.h>
#endif /* HAVE_UTIL_H */
-#endif /* HAVE_LIBUTIL_H */
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.
There's an issue and pull request for it, if that's merged we can use upstream.
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.
Thanks, I have found the issue: https://bugs.python.org/issue34027
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.
And the PR: python/cpython#8056
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 you include the link to the pull request as comment, so it is easier to track, when it is merged?
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 opened this in preparation of #42777, it's not urgent until that's merged. I'll update it with the relevant information when upstream makes a decision.
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.
Updated to use @orivej's patch, since that's most likely to get accepted upstream.
Success on x86_64-linux (full log) Attempted: python37 Partial log (click to expand)
|
Success on aarch64-linux (full log) Attempted: python37 Partial log (click to expand)
|
Success on x86_64-darwin (full log) Attempted: python37 Partial log (click to expand)
|
Thank you! |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)