Skip to content

py: Rename sys module to usys. #6164

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

Closed
wants to merge 1 commit into from
Closed

Conversation

stinos
Copy link
Contributor

@stinos stinos commented Jun 18, 2020

This is consistent with the other 'micro' modules and allows
implementing additional features via e.g. micropython-lib's sys.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Jun 18, 2020
@dpgeorge
Copy link
Member

dpgeorge commented Jun 18, 2020

Thanks, I guess this makes sense to do. But would need to think about it considering the previous discussion (which included sys/usys) in #4370

@stinos
Copy link
Contributor Author

stinos commented Jun 18, 2020

Main reason for this PR is last line in the commit: I came across the need for sys.intern() which can be as simple as a function which just returns it's argument. Seems silly to add that in C, with a flag to enable/disable etc so as far as I know the easiest way to achieve that extend builtin modules is to add a sys.py and in that one do from usys import * then add intern?

From previous discussion it looks like the main argument against doing this is that sys is a 'system' module. But while that is true, library writers etc will use it anyway.

@dpgeorge
Copy link
Member

I came across the need for sys.intern() which can be as simple as a function which just returns it's argument. Seems silly to add that in C, with a flag to enabel/disable etc so as far as I know the easiest way to achieve that extend builtin modules is to add a sys.py and in that one do from usys import * then add intern

Yes, that does make sense, that's what the u-module naming is for (actually the u-module naming is overloaded, it means both "subset of CPy" and "extension of CPy", which is why it's sometimes confusing...).

But note there are quite a few test failures with qemu-arm and minimal unix, and this is because they are not enabling weak-links which would automatically search for usys when importing sys. One fix for that is to enable weak links by default, another is to change all tests so they use usys instead of sys (but it needs try/except to also work with CPython...). Neither of those seem ideal.

@stinos
Copy link
Contributor Author

stinos commented Jun 18, 2020

Neither of those seem ideal.

Not really.. But all other modules like array also use the tests like that, and leaving weak links disabled should stay like that imo (minimalism, plus making sure that also has test coverage). So I'm inclined to change the tests.

@dpgeorge
Copy link
Member

leaving weak links disabled should stay like that imo

Agreed.

But note that this change will break (custom) ports that do not enable weak links and use "import sys". In some places sys is used to detect uPy vs CPy via sys.implementation so now that must become a try to import usys first, which on its own I guess could distinguish between uPy and CPy.

It would be easy to add a special weak link for sys->usys which is always enabled, and that would retain backwards compatibility (import sys still works). But that seems a bit messy.

So I'm inclined to change the tests.

Probably this is the right approach. There'll also need to be a notice in the release notes that sys is now gone...

@stinos stinos force-pushed the usys branch 8 times, most recently from 38ae8b7 to c48d812 Compare August 24, 2020 15:43
@stinos
Copy link
Contributor Author

stinos commented Aug 24, 2020

I've updated this now and changed to usys in all places (nearly all tests), except

  • code which is to be ran strictly under (non-Micro)Python
  • code which already uses one of the other weakly linked modules.
  • the settrace tests since they rely on line numbers
  • the cpydiff tests for clarity (otherwise it reads like 'obviously this is a CPython difference, it imports usys which CPython doesn't have')
  • unrelated documentation like docs/reference/mpyfiles.rst

Code which is meant to be ran using MicroPython will import usys directly whereas cross-platform code will resolve to import usys as sys.

@stinos
Copy link
Contributor Author

stinos commented Aug 25, 2020

Note: no idea why the usyncaio_fair test fails. It doesn't seem to reach the after_failure stage, or that is skipped, so test failures are not printed (which is a problem by itself) and I cannot reproduce it locally nor on my own Travis builds.

@dpgeorge
Copy link
Member

no idea why the usyncaio_fair test fails

It's probably related to the tests added in 20948a3 and/or 55c76ea .

@dpgeorge
Copy link
Member

Rerunning the Travis job made it pass...

@dpgeorge
Copy link
Member

See #6374 for a proposed fix to the uasyncio_fair test.

@dpgeorge
Copy link
Member

uasyncio_fair test fix is pushed to master, so this could be rebased.

This is consistent with the other 'micro' modules and allows
implementing additional features via e.g. micropython-lib's sys.
Note this change is not backwards compatible for ports which do
not enable weak links as they used to be able to import sys but
won't be able to do that anymore now, and need to use import usys.
@dpgeorge
Copy link
Member

I'm happy to make this change but IMO it should wait until after the impending v1.13 release.

@stinos
Copy link
Contributor Author

stinos commented Aug 27, 2020

Sure, no problem.

@peterhinch
Copy link
Contributor

For cross-platform code to run on official firmware on ESPx or Pyboard, on the Unix build, or under CPython I use this:

import sys
if sys.implementation.name == 'micropython':
    # Running MicroPython
else:
    # Running CPython

Will this continue to work on Unix/hardware ports courtesy of weak links?

@stinos
Copy link
Contributor Author

stinos commented Aug 27, 2020

This will keep on working for ports which have weak links enabled, and will fail with an ImportError for others.

I get that this is a drastic change, but, as usual, hard to tell how much code is actually going to be affected?

@robert-hh
Copy link
Contributor

IMHO quite a lot. So what is the strong reason for the change?

@stinos
Copy link
Contributor Author

stinos commented Aug 27, 2020

Consistency with all other u-modules and being able to extend the sys module by having a sys.py, instead of having to extend it in C (also see comments at the beginning of this thread).

From a quick search it looks like these ports are affected i.e. don't have weak links enabled:

  • bare-arm
  • mimxrt
  • minimal
  • pic16bit
  • powerpc
  • qemu-arm
  • samd
  • teensy

@dpgeorge
Copy link
Member

dpgeorge commented Sep 3, 2020

Ok, merged in 40ad8f1

All of the main "production" ports should be OK, code should run unchanged on them. If issues come up we can address them.

@dpgeorge dpgeorge closed this Sep 3, 2020
@stinos
Copy link
Contributor Author

stinos commented Sep 3, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants