bpo-32493: Fix the detection of uuid_enc_be() function of libuuid in configure. by aixtools · Pull Request #7511 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Using git bisect - I found the issue that prevents _uuid from building on AIX.
Short explanation: the logic in configure is always true, so HAVE_UUID_ENC_BE gets defined and the link of the module fails.
5734f41 is the first bad commit
commit 5734f41
Author: Miss Islington (bot) 31488909+miss-islington@users.noreply.github.com
Date: Thu May 24 16:22:59 2018 -0700
bpo-32493: Fix uuid.uuid1() on FreeBSD. (GH-7099)
Use uuid_enc_be() if available to encode UUID to bytes as big endian.
(cherry picked from commit 17d8830312d82e7de42ab89739b0771f712645ff)
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
:040000 040000 6983052a6c16a7fda20a50595a25b8bd28b3e5d6 4c011f4015cbdc47ebfd003fa111b869227235cd M Misc
:040000 040000 9bd35582651acc82fdb7be7100d33843a39fb0b4 5c71739d2798ecf74076e172ddfaf0478f4b92de M Modules
:100755 100755 f1f01df4a80e4262d0c849aa467dfb7afec7df66 b1cd944ac562ea363eb8af040d280e1ec59c03c0 M configure
:100644 100644 2535969642042f38ad4a4a7ef485f9aef94f7b5a 59489047fc24eb1b0fe2dc2d642bd097226235c5 M configure.ac
:100644 100644 a0efff9777d27d53086971543e81abf4604a1bf6 e561a7c07cca7896f35faaa1f2dc9d07823ca42a M pyconfig.h.in
https://bugs.python.org/issue32493
@aixtools Can you submit the PR against the "master" branch, please? Normally, all PRs are targeted for there and then, as necessary, if a core developer decides to merge it, they will backport it to other branches like 3.7. Also, include @serhiy-storchaka on the review.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I fixed similar bugs in past, but added incorrect logic for uuid_enc_be.
The PR LGTM except a news entry.
| @@ -0,0 +1 @@ |
|---|
| correct logic in configure.ac so that HAVE_UUID_ENC_BE is not always defined |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please start the sentence from the title case, and add a period at the end.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The period at the end - I understand, but now clue what you mean about starting the sentence from the title case.
Happy to change the text - but is it really so different from the others? I am not the only one to have not ended the string with a period.
Now it just takes another 24 hours before anything happens - you answer, I find time to change it, the PR gets updated, run through the verification, and then maybe it gets merged - and this breaks - for AIX - what the prior PR was fixing.
Ned - I may have misunderstood re: master and 3.7, but I thought, in emails with victor - a quick PR based on 3.7 was okay (if not preferred). This is so I can back on another PR for uuid and AIX that won't work until this merged.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By “title case”, I think he means start the sentence with a capital letter: “correct logic” becomes “Correct logic”
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike the commit title and the commit message. You should write something simpler like:
bpo-32493: Fix _uuid on AIX
Fix the detection of uuid_enc_be() function of libuuid in configure.
What you write as description is fine for a bug description, but not for a commit message. As I wrote in https://bugs.python.org/issue32493#msg319025 I suggest you to open a new issue specific to AIX.
Apart of that, I concur with Serhiy, the configure change LGTM.
Thank you for your time.
I hope the commit changes resolve the remaining issues.
Fix the detection of uuid_enc_be() function of libuuid in configure.
bpo-32493: correct logic in configure.ac so that HAVE_UUID_ENC_BE is not always … bpo-32493: Fix the detection of uuid_enc_be() function of libuuid in configure.
ned-deily pushed a commit to ned-deily/cpython that referenced this pull request
ned-deily added a commit that referenced this pull request
@aixtools, Thanks for all of youir work on this and sorry about the confusion! To expedite things because of the approaching 3.7.0rc1 deadline, I pushed a few changes to your PR and then merged it to the 3.7 branch. I also cherry-picked the changed into a new PR #7567 for the master (e.g. 3.8) branch and merged that.
For future reference, when submitting PRs, please make them against the master branch (unless the problem does not also exist there), and not 3.7 like was done here. If a core developer decides to merge the PR into master, they will also decide to what other branches, if any, the PR should be backported and then we have an automated workflow to handle the backport. Thanks!
Glad it helps. No worries about my learning experience.