Skip to content

bpo-34579: Modify test_embed DEFAULT_CONFIG for AIX#9063

Merged
vstinner merged 7 commits intopython:masterfrom
aixtools:bpo-34579
Sep 15, 2018
Merged

bpo-34579: Modify test_embed DEFAULT_CONFIG for AIX#9063
vstinner merged 7 commits intopython:masterfrom
aixtools:bpo-34579

Conversation

@aixtools
Copy link
Copy Markdown
Contributor

@aixtools aixtools commented Sep 4, 2018

@aixtools
Copy link
Copy Markdown
Contributor Author

aixtools commented Sep 4, 2018

not sure what is wrong with my filename and bedevere/news checking.

Both: Misc/NEWS.d/next/Tests/2018-09-04-15-10-05.bpo-34579.xogER2.rst and 2018-09-04-15-16-42.bpo-34579.bp4HdM.rst have been rejected as filenames.

Does it not like GMT timestamped files? Does it think mine are in the future? The 'details' only points at the documentation about the filename, not why it was rejected. Not going to try a third name.

b) more "unluck":
image

Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Please don't duplicate the whole DEFAULT_CONFIG, it would be annoying to maintain these two copies.

IHMO the change is wrong. Why would AIX behave differently than other operating systems?

@bedevere-bot
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.

@aixtools
Copy link
Copy Markdown
Contributor Author

Will be a few days before I have time to make the change.

As to why AIX is different from other platforms:
a) no clue why other platforms print "(null)" when there is nothing.
b) I have not been able to locate the precise code that generates this output. If you can point me to it maybe I can figure out why they differ.

Thanks for the review.

@vstinner
Copy link
Copy Markdown
Member

a) no clue why other platforms print "(null)" when there is nothing.

You cannot come with a solution if you didn't identify the root issue.

@vstinner
Copy link
Copy Markdown
Member

b) I have not been able to locate the precise code that generates this output. If you can point me to it maybe I can figure out why they differ.

See Programs/_testembed.c. You can run the program directly.

@aixtools
Copy link
Copy Markdown
Contributor Author

It seems there is a difference is the way AIX libc deals with printing NULL:

root@x066:[/data/prj/python]cat nullpr.c
#include <stdio.h>
main()
{
char *str = NULL;

    printf("NULL prints as:>>>%s<<<\n", str);

}

root@x066:[/data/prj/python]uname
AIX

root@x066:[/data/prj/python]./nullpr
NULL prints as:>>><<<

root@x074:/data/prj/python# uname
Linux
root@x074:/data/prj/python# ./nullpr
NULL prints as:>>>(null)<<<
root@x074:/data/prj/python#

@aixtools
Copy link
Copy Markdown
Contributor Author

aixtools commented Sep 13, 2018

Note: there is a failed test, but not in anything I have changed:

root@x066:[/data/prj/python/git/python3-3.8]git branch

  • bpo-34579
    bpo-pr
    master
    root@x066:[/data/prj/python/git/python3-3.8]git diff master Lib/test/test_socket.py
    root@x066:[/data/prj/python/git/python3-3.8]

I have made the requested changes; please review again.

update: removed reference to issue/bpo 1 1 9 9 2, so that maybe the PR reference in that issue also goes away.

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I would prefer to see a constant NULL_STR which would be equal to "(null)" on Linux and "" on AIX.

@bedevere-bot
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.

And if you don't make the requested changes, you will be poked with soft cushions!

@aixtools
Copy link
Copy Markdown
Contributor Author

excellent idea. Wish it had been mine!

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Comment thread Lib/test/test_embed.py Outdated
Comment thread Misc/NEWS.d/next/Tests/2018-09-04-15-16-42.bpo-34579.bp4HdM.rst Outdated
@aixtools
Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link
Copy Markdown

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

@vstinner vstinner merged commit d206731 into python:master Sep 15, 2018
@vstinner
Copy link
Copy Markdown
Member

Oh no. I was editing the commiting the commit message on my phone when my phone decided that it should be merged with the incomplete commit message :-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants