Skip to content

bpo-21263: Skip test_gdb when python has been compiled with LLVM clang #10318

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 4, 2018
Merged

bpo-21263: Skip test_gdb when python has been compiled with LLVM clang #10318

merged 1 commit into from
Nov 4, 2018

Conversation

lysnikolaou
Copy link
Member

@lysnikolaou lysnikolaou commented Nov 4, 2018 •
edited by bedevere-bot
Loading

I have also added a new field in the test support module, which enables one to find out which compiler was used to build Python.

https://bugs.python.org/issue21263

@ned-deily
Copy link
Member

Do we foresee any other uses for such a utility function? If not, I'd prefer just a direct check in test_gdb.

@lysnikolaou
Copy link
Member Author

lysnikolaou commented Nov 4, 2018 •
edited
Loading

Do we foresee any other uses for such a utility function? If not, I'd prefer just a direct check in test_gdb.

I thought it might be helpful in the future.
One other thing is that I would have to import platform in test_gdb, but it is already imported in the support module, so I didn't know if it was okay to import it once more.

@ned-deily
Copy link
Member

Importing again is fine. That's the way Python works! While it is often a good idea to not repeat yourself, in this case the test is so simple that it is almost more work to create and document a utility function. I would leave it out.

@lysnikolaou
Copy link
Member Author

Ok then. I updated my PR. Thanks a lot for your help.

@lysnikolaou
Copy link
Member Author

Should I maybe include a news entry for this? I have never done so in the past, but I am not sure about this PR.

ned-deily
ned-deily requested changes Nov 4, 2018
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

With regard to a NEWS entry, since there are already two bpo issues that have been opened about this, I suppose it makes sense to include one. If you feel comfortable doing so, you can use the blurb tool to do so. Otherwise, the core committer making the final merge could do it. Thanks!

Lib/test/test_gdb.py Outdated
@@ -48,6 +49,10 @@ def get_gdb_version():
if not sysconfig.is_python_build():
raise unittest.SkipTest("test_gdb only works on source builds at the moment.")

if 'Clang' in platform.python_compiler():
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't notice this before but the skip should be limited to macOS, i.e. add a sys.platform == "darwin" clause to the if test. AFAICT gdb does work OK with clang on Linux for example.

Lib/test/test_gdb.py Outdated
@@ -48,6 +49,10 @@ def get_gdb_version():
if not sysconfig.is_python_build():
raise unittest.SkipTest("test_gdb only works on source builds at the moment.")

if 'Clang' in platform.python_compiler():
raise unittest.SkipTest("test_gdb doesn't work correctly, when python is"
Copy link
Member

Choose a reason for hiding this comment

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

No need for the comma (,) there.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@lysnikolaou
Copy link
Member Author

I have made the requested changes; please review again

I have added a news entry, but I am not exactly sure about the wording.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ned-deily: please review the changes made to this pull request.

ned-deily
ned-deily approved these changes Nov 4, 2018
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! If you like, you can add your name to Misc/ACKS and to the NEWS entry: Patch by ....

@lysnikolaou
Copy link
Member Author

lysnikolaou commented Nov 4, 2018 •
edited
Loading

Can I directly change the NEWS entry or should I somehow use blurb to do so?

@ned-deily
Copy link
Member

Either way is fine.

@lysnikolaou
Copy link
Member Author

I just committed one last time to update Misc/ACKS and the news entry. Thanks a lot for the review and all your help.

@ned-deily ned-deily merged commit 59668aa into python:master Nov 4, 2018
@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-10325 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 4, 2018
@lysnikolaou @miss-islington
bpo-21263: Skip test_gdb when python has been compiled with LLVM clang (
904babb 
pythonGH-10318)

(cherry picked from commit 59668aa)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
@miss-islington
Copy link
Contributor

Thanks @lysnikolaou for the PR, and @ned-deily for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@lysnikolaou lysnikolaou deleted the bpo21263 branch November 4, 2018 21:23
@bedevere-bot
Copy link

GH-10326 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 4, 2018
@lysnikolaou @miss-islington
bpo-21263: Skip test_gdb when python has been compiled with LLVM clang (
26847fe 
pythonGH-10318)

(cherry picked from commit 59668aa)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
ned-deily pushed a commit that referenced this pull request Nov 4, 2018
@miss-islington @lysnikolaou @ned-deily
bpo-21263: Skip test_gdb when python has been compiled with LLVM clang (
96fb350 
GH-10318) (GH-10325)

(cherry picked from commit 59668aa)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
ned-deily pushed a commit that referenced this pull request Nov 4, 2018
@miss-islington @lysnikolaou @ned-deily
bpo-21263: Skip test_gdb when python has been compiled with LLVM clang (
f574ce7 
GH-10318) (GH-10326)

(cherry picked from commit 59668aa)

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
@sorcio sorcio mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ned-deily ned-deily ned-deily approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lysnikolaou @ned-deily @bedevere-bot @miss-islington @the-knights-who-say-ni