Skip to content
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

Pass and use original sys.argv to/with workers #388

Merged
merged 1 commit into from Dec 12, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Dec 9, 2018

This gets used e.g. by argparse for the "prog" part.

We could explicitly pass it through and/or set it on
config._parser.prog, but that is a bit tedious just for this use case,
and it looks like "simulating" the main prog here appears to not be that
bad of a hack after all.

Fixes #358, Fixes #384.

@blueyed
Copy link
Contributor Author

blueyed commented Dec 9, 2018

Noticed this with pytest-dev/pytest#4523 - it uses "-c" otherwise.

@RonnyPfannschmidt
Copy link
Member

hm, this hack is a double edged sword, we should reset that stuff after the config initialization is sorted out correctly

@nicoddemus
Copy link
Member

nicoddemus commented Dec 9, 2018

This also should fix #384 (updated the description to reflect that).

@nicoddemus
Copy link
Member

Changing sys.argv like this makes me a little uncomfortable, but it seems the only way to make this work if users want to peek at sys.argv in worker processes anyway.

@RonnyPfannschmidt
Copy link
Member

again - this hack is going to be a problem ^^

@nicoddemus
Copy link
Member

hm, this hack is a double edged sword, we should reset that stuff after the config initialization is sorted out correctly

Oh, to me this reads as "Well this is problematic, we should revert this when config is fixed", so I understood it was OK to merge this.

Well, then we might as well say that workers don't have access to the same sys.argv of the master process then. Seems like the correct approach given that workers are started with a different and undefined sys.argv and shouldn't be relied on. We could then add this to the FAQ.

@nicoddemus
Copy link
Member

Hah, coincidentally just ran into this post by @nedbat! 😁

@nicoddemus
Copy link
Member

nicoddemus commented Dec 9, 2018

actually, probably this patch would make his error appear with xdist as well.

@blueyed
Copy link
Contributor Author

blueyed commented Dec 10, 2018

We could also make "mainargv" available somehow.

My use case was getting proper "prog" with the usage information in pytest, and that would at least require to pass through "prog" quite a bit - or set it on _parser directly, which also did not feel right.
Would you like that better, @RonnyPfannschmidt ?

@nicoddemus
Copy link
Member

Would just document that master's sys.argv is available in config.workerinput["mainargv"] is enough? If yes, you only need to remove the sys.argv assignment.

@blueyed
Copy link
Contributor Author

blueyed commented Dec 10, 2018

Yeah, that might be good enough.
And I assume the pytest plugin part could actually set something on _parser directly then from there? (otherwise it would also require a patch to pytest, but it might be good after all to handle prog there in a better way)

@nicoddemus
Copy link
Member

And I assume the pytest plugin part could actually set something on _parser directly then from there?

Hmm I don't know about the _parser bit, can you elaborate?

@nicoddemus
Copy link
Member

Ahh OK, that's the "use" bit in the title of the PR.

I think this can be postponed to another PR, as it seems quite tricky to get it right, while just making mainargv available is already useful for some users.

@blueyed
Copy link
Contributor Author

blueyed commented Dec 11, 2018

@RonnyPfannschmidt
Do you agree?

How is workerinput used typically / after all?

@blueyed blueyed force-pushed the pass-mainargv branch 3 times, most recently from 31955ea to e4d692c Compare December 11, 2018 05:06
@blueyed
Copy link
Contributor Author

blueyed commented Dec 11, 2018

Done.
But could not find a place to document it, added a changelog though.

This gets used e.g. by argparse for the "prog" part.

We could explicitly pass it through and/or set it on
config._parser.prog, but that is a bit tedious just for this use case,
and it looks like "simulating" the main prog here appears to not be that
bad of a hack after all.

def test_remote_usage_prog(testdir, request):
if not hasattr(request.config._parser, "prog"):
pytest.skip("prog not available in config parser")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus
Copy link
Member

But could not find a place to document it, added a changelog though.

I suggest to add a new section before/after "Identifying the worker process during a test" in the README.

@nicoddemus
Copy link
Member

I created a follow up for the docs: #391

Merging now as the failure is unrelated and has been fixed on master already. 👍

Thanks @blueyed!

@nicoddemus nicoddemus merged commit 95aaaec into pytest-dev:master Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants