Enable requiring zipped modules ENG-168 - #287
Conversation
| repo_path = os.path.join(root, subfolder) | ||
| if not os.path.isdir(repo_path): | ||
| raise InvalidModuleArchiveError( | ||
| f"Module archive '{archive_path}' is missing the '{subfolder}/' folder at its root. " |
There was a problem hiding this comment.
Here we require a MODULE_TESTS_SUBFOLDER to exist. This might be an issue in the case where we want to make a .module file when rendering something that has conformance tests turned off (by not supplying a conformance test script).
There was a problem hiding this comment.
Good point. I added a function archive_has_conformance_tests in plain_modules that checks if the .module file has tests. If it doesn't but the config file enables conformance testing, the user gets presented two options in the TUI: quit or rerender the module and delete the .module file.
| shutil.rmtree(self.module_folder) | ||
| if os.path.isdir(self._default_module_folder): | ||
| console.warning(f"Wiping module {self._default_module_folder}...") | ||
| shutil.rmtree(self._default_module_folder, ignore_errors=True) |
There was a problem hiding this comment.
Not really expecting shutil.rmtree to fail too often but if it does it now fails silently. It would just say "wiping module" and then nothing if it failed which might not be ideal.
There was a problem hiding this comment.
Good catch. That's definitely not optimal.
There was a problem hiding this comment.
This should use the file_utils anyway. Fixed this.
| serializable: dict = {} | ||
| for frid, entry in conformance_tests_json.items(): | ||
| if isinstance(entry, dict) and "folder_name" in entry: | ||
| entry = {**entry, "folder_name": os.path.relpath(entry["folder_name"], base)} |
There was a problem hiding this comment.
Do we want to support transferring of modules from Windows to Linux? Just a thought but operating with os.path.relpath and other OS-reliant operations could be an issue and should be tested to make sure the handling of things like different separators works.
There was a problem hiding this comment.
No, it's not a requirement at the moment to support transferring of modules from Windows to Linux.
VitjanZ
left a comment
VitjanZ
left a comment
There was a problem hiding this comment.
I put some minor things as comments so you can consider them. Otherwise I tested it and seems to work fine. Also ran the branch on the benchmark to see if standard examples pass without issues and they do.
@VitjanZ Thanks for the benchmark run. I need to get into the habit of doing that. I made two changes:
|
96e1f69 to
c8de6aa
Compare
3 participants
Purpose
Let a rendered module's build output be distributed and consumed as a single
<module>.modulezip archive instead of an unpackedplain_modules/<module>/folder.Changes and design decisions
PlainModule.materialize()extracts an archive-only module to a scratch temp dir for read-only consumption (the archive is kept and no folder is created);ensure_module_unpacked()unpacks in place and deletes the archive when a module is (re)rendered. Reading from scratch honors "don't unzip" for consumption; unpack-in-place happens only on change.materialize()over the whole module tree at render start, with scratch cleanup inmain()'sfinally. Chosen over a lazy side-effecting property so timing is deterministic (before change detection and beforeRenderContextfreezes the build path) and cleanup is reliable on every exit path.ConformanceTestsresolves each module's tests folder through an injected resolver, so archived required modules resolve to their scratch extraction.conformance_tests.jsonstoresfolder_namerelative to the module's tests folder (resolved to absolute on read). This is scratch-safe and portable.tests/is optional in an archive. A module rendered without a conformance-tests script has notests/folder, so its.modulehas none either. Validation requires onlycode/(with.git);tests/is validated only when present. When conformance testing IS enabled but a consumed archive has notests/, the module cannot be used as-is: it surfaces amissing_conformance_testsrender-state reason (a hard blocker that takes precedence over spec/code changes), and the user is warned and offered quit / rerender-that-module. On rerender the module rebuilds (code + tests) and its.moduleis removed; on quit the archive is left untouched.code/(andtests/when present) with.git, zip-slip guard, exec-bit restore. AddsInvalidModuleArchiveError.Breaking changes
The module code-hash format changed to content-only. Existing multi-module projects see a one-time "required module code changed" prompt on their next render, and any
.modulearchives created before this change must be regenerated (their stored hashes embed the old absolute path).Testing
code/, missing.git, zip-slip), folder-vs-archive parity, relative-path round-trip, clone from an archived required module, location-independent code hash, code-only (tests-less) archive validate / materialize / unpack, and detection + render-choice for a tests-less archive when conformance is enabled..modulefiles to a deeper directory and rendered successfully; changed the spec of a zipped module and confirmed correct render choices.