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

contextlib: (re) add ExitStack? #49

Open
dxxb opened this issue Oct 19, 2015 · 15 comments
Open

contextlib: (re) add ExitStack? #49

dxxb opened this issue Oct 19, 2015 · 15 comments
Labels
needs-info This issue needs more information to be resolvable

Comments

@dxxb
Copy link
Contributor

dxxb commented Oct 19, 2015

ExitStack was added in 5557382 from CPython 3.4.2 and subsequently removed in 92ef77c maybe because it wasn't compatible with uPy out of the box.

I am using a slightly modified ExitStack taken from CPython lib for some unittests which need to run on both my development host and on the target device and I am wondering if there is any interest in having ExitStack added to contextlib. Thanks

@dpgeorge
Copy link
Member

I'd say it was removed simply because it didn't run out-of-the-box, and there was no need at the time to get it working.

@pfalcon
Copy link
Contributor

pfalcon commented Oct 19, 2015

What seems as "removal" is actually following guidelines of https://github.com/micropython/micropython-lib/wiki/ContributorGuidelines ("When porting a module from CPython or other 3rd-party source" section). So, it wasn't removed, it just "wasn't added" to micropython-lib by the original submitter.

@pfalcon
Copy link
Contributor

pfalcon commented Oct 19, 2015

Some random collection of comments regarding ExitStack:

  1. This is the first time I hear about it.
  2. Does anybody use it? Yeah, I figure you do, but does anybody use it in the sense of https://github.com/micropython/micropython/wiki/ContributorGuidelines#django-criteria , i.e. is there well established codebase which is useful to run on uPy, which uses ExitStack, and where ExitStack is few of missing features required to run it?
  3. contextlib is one of those modules which is useful for PyBoard, so I'd like to keep it small, and from patches you quote, ExitStack is huge.

Actually, I'd like to keep it minimal, and current contextlib.py is already too big, and full of comments and stuff. Solve-it-all solution would be to create ucontexlib.py (in the own module of course), and let it be truly minimal (e.g. start with just contextmanager). Then contextlib.py can be fully CPython compatible (either importing ucontexlib, or just being completely separate, depending on what's easier to maintain).

This still leaves questions of why you use "slightly modified" version, instead of verbatim CPython version, and how to be about tests (as I mentioned, if something tries to be ~fully CPython compatible, it should also take tests from CPython and pass them).

So, if you'd be willing to resolve all these issues, it would be very welcome!

@dxxb
Copy link
Contributor Author

dxxb commented Oct 20, 2015

Hi,

Starting from @pfalcon's last question: ExitStack code from CPython does not run verbatim see dxxb@9c492ee for the (very limited) changes I made.

Yes, ExitStack is large and I didn't know about it either until I went looking for the best way to enter context managers conditionally see the discussion at http://bugs.python.org/issue13585.

I agree we should go with your solve-it-all solution. I would submit patches to spin off a ucontextlib.py module with just contextmanager and make the current contextlib depend on it then add ExitStack.

CPython tests are an issue, they will not work with uPy because they rely an exception instance to have a context attribute and use it like this (taken from https://hg.python.org/cpython/file/3.4/Lib/test/test_contextlib.py)

...
        try:
            with ExitStack() as stack:
                stack.enter_context(gets_the_context_right(exc4))
                stack.enter_context(gets_the_context_right(exc3))
                stack.enter_context(gets_the_context_right(exc2))
                raise exc1
        except Exception as exc:
            self.assertIs(exc, exc4)
            self.assertIs(exc.__context__, exc3)
            self.assertIs(exc.__context__.__context__, exc2)
            self.assertIs(exc.__context__.__context__.__context__, exc1)
            self.assertIsNone(
                       exc.__context__.__context__.__context__.__context__)
...

@dpgeorge
Copy link
Member

ExitStack code from CPython does not run verbatim see dxxb/micropython-lib@f16304f for the (very limited) changes I made.

I see that the only changes (sans white space) are to accommodate the fact that uPy doesn't support setting attributes on functions. I've come across this before trying to port some things. It's pretty expensive to allow writable attributes on functions....

@pfalcon
Copy link
Contributor

pfalcon commented Oct 20, 2015

@dxxb : Sounds good, thanks!

@dxxb
Copy link
Contributor Author

dxxb commented Nov 3, 2015

There is another couple of context managers in contextlib that I would find useful when using unittest on my target: redirect_stdout() and redirect_stderr(). I am currently overriding the print() builtin and writing to an IOString but I'd rather redirect std[out, err]. However, sys.stdout cannot be set. Would it be acceptable to make sys.stdout writable for the unix port?

@pfalcon
Copy link
Contributor

pfalcon commented Nov 3, 2015

I believe I worked on that, but can't see results of that work. Yes, it should be possible to override stdout/stderr on unix. If you think you can figure the machinery needed for this, you're welcome to look into that.

@dxxb
Copy link
Contributor Author

dxxb commented Nov 4, 2015

Judging by this comment I need to look into making stdin/stdout/stderr r/w in modsys.c. I'll give it a shot.

@dpgeorge
Copy link
Member

dpgeorge commented Nov 4, 2015

I need to look into making stdin/stdout/stderr r/w in modsys.c. I'll give it a shot.

This is definitely a feature to add, but it's hard. The only way I can think to do it is something similar to MICROPY_CAN_OVERRIDE_BUILTINS, in objmodule.c.

@pfalcon
Copy link
Contributor

pfalcon commented Nov 4, 2015

The only way I can think to do it is something similar to MICROPY_CAN_OVERRIDE_BUILTINS, in objmodule.c.

To give some context to all readers, I already proposed @dpgeorge to generalized MICROPY_CAN_OVERRIDE_BUILTINS support to any module, and he said he didn't see that having a good benefits/drawbacks ratio.

@pfalcon
Copy link
Contributor

pfalcon commented Nov 4, 2015

Ok, I dug out my WIP in a stash and post it here for reference: pfalcon/pycopy@38300fe . As can be seen I do implement override support for any module. And sys.stdout is actually a hard case, as we store references to it in internal data structures. So, patch adds support for "module .setattr callbacks" to be able to handle such cases.

@dxxb
Copy link
Contributor Author

dxxb commented Nov 6, 2015

Thank you @pfalcon, I am going to look into this as soon as I have the chance (probably next week).

@dpgeorge
Copy link
Member

dpgeorge commented Nov 9, 2015

@pfalcon that patch is quite nice! Just a few issues with root pointers.

@jonnor
Copy link

jonnor commented Aug 25, 2024

It has been 9 years since the last comment. Is this still relevant?

@jonnor jonnor added the needs-info This issue needs more information to be resolvable label Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-info This issue needs more information to be resolvable
Projects
None yet
Development

No branches or pull requests

4 participants