Skip to content

py/frozenmod.c: Use ".frozen/" as a virtual search path for frozen modules. - #8079

Closed
jimmo wants to merge 8 commits into
micropython:masterfrom
jimmo:dot-frozen-sys-path
Closed

py/frozenmod.c: Use ".frozen/" as a virtual search path for frozen modules.#8079
jimmo wants to merge 8 commits into
micropython:masterfrom
jimmo:dot-frozen-sys-path

Conversation

@jimmo

@jimmo jimmo commented Dec 11, 2021

Copy link
Copy Markdown
Member

The current behavior for module searching is:

for prefix in sys.path:
  path = join(p, m)
  # try to find a frozen file `path`
  # try `path` in the filesystem

This meant that frozen files would be found if sys.path contained the empty string (this is the default in most cases), and would always take precedence over the filesystem. Additionally, when running the Unix port with a script in argv, sys.path will contain the path of the script, rather than the empty string). (This prevents uasyncio from being used without sys.path manipulation -- see #6419)

This is an updated version of #5057 by @wnienhaus (and #8000 by @andrewleech ) which add a new "virtual" path to sys.path .frozen which explicitly means "search the frozen modules". This solves both issues:

  • ".frozen" can be placed in any order in sys.path relative to "" allowing the precedence for frozen modules to be set.
  • The Unix port can add ".frozen" separate to "the path that the current script resides in".

It also matches the current CircuitPython behavior (which also uses .frozen as the special directory name).

Relative to #5057 (and #8000) there are a few changes:

  • rebase on the latest changes to builtinimport.c (py/builtinimport.c: Refactor module importing. #8010)
  • avoid allocating additional qstr's for the frozen path (path/foo.py is already a qstr in frozen_content.c, avoid also creating .frozen/path/foo.py)
  • merge frozen string and mpy path names into the same list in frozen_content.c to simplify py/frozenmod.c (and corresponding changes to makemanifest.py and mpy-tool.py).
  • deprecate uio.resource_stream (instead of updating it to match)
  • minor fix to the cmake rules to make the QSTR__dot_frozen in qstrdefs.h work (Fix importing of frozen modules (fixes #2322) #5057 worked around this in a different way)

The main idea of the frozen_content.c change was to reduce code size, although the result isn't huge it almost covers this change. Overall this PR is +16 bytes on PYBV11.

The frozen_content.c change also meant removing non-manifest freezing (via FROZEN_DIR or FROZEN_MPY_DIR) which has been deprecated for two years. Updated the last remaining port (teensy) to use manifest.py.

Fixes #6419, #1804, #2322, #3509

jimmo added 4 commits December 11, 2021 23:05
@jimmo
py/modio.c: Remove io.resource_stream function.
84e9340 
Not enabled on any port, and functionality is better suited to the
micropython-lib implementation of pkg_resources.
@jimmo
teensy: Use manifest.py instead of FROZEN_DIR.
111a828 
Signed-off-by: Jim Mussared <[email protected]>
@jimmo
top: Remove support for FROZEN_DIR, FROZEN_MPY_DIR.
6e22753 
This has been deprecated for over two years in favour of manifest.py.

Signed-off-by: Jim Mussared <[email protected]>
@jimmo
tools/makemanifest.py: Merge make-frozen.py.
0ca1947 
Takes the functionality from tools/make-frozen.py, adds support for
multiple frozen directories, and moves it to tools/makemanifest.py.

Signed-off-by: Jim Mussared <[email protected]>
@jimmo
jimmo force-pushed the dot-frozen-sys-path branch from ad63c59 to 286f091 Compare December 11, 2021 14:03
@jimmo

jimmo commented Dec 11, 2021

Copy link
Copy Markdown
Member Author

Updated to completely merge mp_frozen_stat with mp_find_frozen_module. Overall code size is -ve on minimal, and -84 bytes on PYBV11.

@jimmo
jimmo force-pushed the dot-frozen-sys-path branch from 286f091 to 8708342 Compare December 11, 2021 14:09
@codecov-commenter

codecov-commenter commented Dec 11, 2021 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.46%. Comparing base (0892ebe) to head (f1470c3).
⚠️ Report is 5400 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8079      +/-   ##
==========================================
- Coverage   98.47%   98.46%   -0.02%     
==========================================
  Files         153      153              
  Lines       20176    20135      -41     
==========================================
- Hits        19869    19825      -44     
- Misses        307      310       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jimmo
jimmo force-pushed the dot-frozen-sys-path branch 4 times, most recently from 5e095d5 to f1470c3 Compare December 12, 2021 00:02
jimmo added 4 commits December 12, 2021 11:03
@jimmo
py/frozenmod: Only search .frozen/ path.
6234959 
This changes makemanifest.py & mpy-tool.py to merge string and mpy names
into the same list (now mp_frozen_names).

The various paths for loading a frozen module(mp_find_frozen_module) and
checking existence of a frozen module (mp_frozen_stat) use a common
function that searches this list.

In addition, the frozen lookup will now only take place if the path
starts with ".frozen", which needs to be added to sys.path.

Signed-off-by: Jim Mussared <[email protected]>
@jimmo
py/mkrules.cmake: Set frozen defs early.
f7b3751 
This ensures MICROPY_QSTR_EXTRA_POOL and MICROPY_MODULE_FROZEN_MPY
are set if necessary before the CFLAGS are extracted for QSTR
generation.

Signed-off-by: Jim Mussared <[email protected]>
@jimmo
ports: Add .frozen as first entry in sys.path.
d833f02 
Frozen modules will be searched preferentially, but gives the
user the ability to override this behavior.

This matches the previous behavior where "" was implicitly the
frozen search path, but the frozen list was checked before the
filesystem.

Signed-off-by: Jim Mussared <[email protected]>
@jimmo
docs/library/sys.rst: Add note about .frozen.
f1470c3 
Signed-off-by: Jim Mussared <[email protected]>
andrewleech
andrewleech reviewed Dec 13, 2021
View reviewed changes
Comment thread ports/unix/main.c
#endif
}
size_t path_num = 1; // [0] is for current dir (or base dir of the script)
size_t path_num = 2; // [0] is frozen, [1] is for current dir (or base dir of the script)

@andrewleech andrewleech Dec 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ordering of putting the script path second should also fix the upip issue, looks good to me.
It will mean however if you run upip from source rather than frozen in, any libs will be installed next to upip.py script by default

Comment thread ports/unix/mpconfigport.h
Comment thread py/builtinimport.c
@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Dec 17, 2021
@dpgeorge

dpgeorge commented Dec 17, 2021

Copy link
Copy Markdown
Member

Thanks for the efforts on this, it fixes a very long standing bug.

Merged in cc23e99 through 86394f7, with follow-up commit in de43b50

I made the following minor changes during merge/rebase:

  • updated docs to specify that .frozen is never searched as a directory
  • make import unconditionally return the result of mp_find_frozen_module() if the path begins with .frozen (as discussed above)
  • removed io.resource_stream tests
  • added back section about FROZEN_MANIFEST to nrf/README.md, because it's still relevant
  • allowed passing NULL to the last 2 params of mp_find_frozen_module() so it doesn't unnecessarily create a lexer if not needed
  • made mp_find_frozen_module() set *frozen_type = MP_FROZEN_NONE; at start in case it returned MP_IMPORT_STAT_DIR (possibly fixes a bug where on bare-metal there could be a frozen directory named main and it tries to import it as a frozen module)

Code size diff of this PR and the follow-up commit:

   bare-arm:   +32 +0.056% 
minimal x86:   +32 +0.020% 
   unix x64:  -304 -0.059% 
unix nanbox:  -340 -0.075% 
      stm32:  -100 -0.025% PYBV10
     cc3200:    +0 +0.000% 
    esp8266:  -204 -0.029% GENERIC
      esp32:   +28 +0.002% GENERIC[incl +16(data)]
        nrf:   -44 -0.025% pca10040
        rp2:   +24 +0.005% PICO
       samd:   -80 -0.057% ADAFRUIT_ITSYBITSY_M4_EXPRESS

The increase on bare-arm and minimal is due to the follow-up commit de43b50 which actually fixes a bug on these ports because they previously did not initialise mp_sys_path or mp_sys_argv (and now they do).

@dpgeorge dpgeorge closed this Dec 17, 2021
This was referenced Dec 17, 2021
Closed
Closed
Merged
@dpgeorge dpgeorge mentioned this pull request Jan 27, 2022
Open
@jimmo jimmo mentioned this pull request Apr 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge left review comments
+1 more reviewer
@andrewleech andrewleech andrewleech left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

py-core Relates to py/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

unix: sys.path affects loading of frozen modules

4 participants

@jimmo @codecov-commenter @dpgeorge @andrewleech