FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-21263: Skip test_gdb when python has been compiled with LLVM clang by lysnikolaou · Pull Request #10318 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Merged
ned-deily merged 1 commit into
python:masterfrom
lysnikolaou:bpo21263
Nov 4, 2018
ConversationCommits (1)ChecksFiles changed
Merged

bpo-21263: Skip test_gdb when python has been compiled with LLVM clang#10318
ned-deily merged 1 commit into
python:masterfrom
lysnikolaou:bpo21263

Conversation

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

Copy link
Copy Markdown
Member

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 commented Nov 4, 2018

Copy link
Copy Markdown
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 commented Nov 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Nov 4, 2018

Copy link
Copy Markdown
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 commented Nov 4, 2018

Copy link
Copy Markdown
Member Author

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

lysnikolaou commented Nov 4, 2018

Copy link
Copy Markdown
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 requested changes Nov 4, 2018
View reviewed changes

ned-deily left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

Comment thread Lib/test/test_gdb.py Outdated

ned-deily Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Lib/test/test_gdb.py Outdated

ned-deily Nov 4, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No need for the comma (,) there.

bedevere-bot commented Nov 4, 2018

Copy link
Copy Markdown

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 commented Nov 4, 2018

Copy link
Copy Markdown
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 commented Nov 4, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

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

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

ned-deily left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lysnikolaou commented Nov 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

ned-deily commented Nov 4, 2018

Copy link
Copy Markdown
Member

Either way is fine.

lysnikolaou commented Nov 4, 2018

Copy link
Copy Markdown
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 merged commit 59668aa into python:master Nov 4, 2018

miss-islington commented Nov 4, 2018

Copy link
Copy Markdown
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 commented Nov 4, 2018

Copy link
Copy Markdown

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
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 commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

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

lysnikolaou deleted the bpo21263 branch November 4, 2018 21:23

bedevere-bot commented Nov 4, 2018

Copy link
Copy Markdown

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
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
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
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 mentioned this pull request Sep 6, 2023
Closed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL