Fix bug in REPL and aggregate duplicated code #157
Conversation
2. Add test for creating a compacted image and opening it in REPL 3. Aggragate duplicate code to functions
|
Hey @amgross! I should find time either tomorrow or this weekend to review! |
|
@BrianPugh reminder |
|
so sorry this slipped through the cracks; looking now 🙈 |
BrianPugh
left a comment
BrianPugh
left a comment
There was a problem hiding this comment.
Added a few small comments, thanks!
src/littlefs/__main__.py
Outdated
| if args.verbose: | ||
| fs_size = len(fs.context.buffer) | ||
| input_image_size = len(fs.context.buffer) |
There was a problem hiding this comment.
I think this is broken:
$ littlefs-python repl -v foo.bin --block-size 512
Traceback (most recent call last):
File "/Users/brianpugh/projects/littlefs-python/.venv/bin/littlefs-python", line 10, in <module>
sys.exit(main())
File "/Users/brianpugh/projects/littlefs-python/src/littlefs/__main__.py", line 364, in main
return args.func(parser, args)
File "/Users/brianpugh/projects/littlefs-python/src/littlefs/__main__.py", line 218, in repl
fs = _mount_from_context(parser, args, context)
File "/Users/brianpugh/projects/littlefs-python/src/littlefs/__main__.py", line 138, in _mount_from_context
input_image_size = len(fs.context.buffer)
AttributeError: 'UserContextFile' object has no attribute 'buffer'
There was a problem hiding this comment.
You are correct of course, fixed and added test to catch it.
I tested those prints with the list/extract CLI which don't have this issue.
| try: | ||
| shell.do_mount() | ||
| except LittleFSError as exc: | ||
| parser.error(f"Failed to mount '{source}': {exc}") |
There was a problem hiding this comment.
we lost this error message in this PR.
src/littlefs/__main__.py
Outdated
| name_max=args.name_max, | ||
| mount=False, | ||
| ) | ||
| shell.cmdloop() |
There was a problem hiding this comment.
do we need a try/finally around this to unmount the filesystem if it's mounted?
|
@BrianPugh any idea why all tests failed/canceled? |
|
it was probably CI fail-early with failed linting. Run |
|
beyond that, i think there's some github actions runner issues at the moment, ill rerun in a bit (I think it's a transient issue). |
2 participants
Bug in REPL
Found bug in repl, that it sends block_count to littlefs based on the file size, but in compacted image the file sze is not really the correct size hence getting error from littlefs.
This is in different than list and extract commands which not sending block_count, and when littlefs getting 0 it ignores it.
(Try run the test file I added with current master and see the failure)
Solution
Make behavior of list+extract+repl same by using same function
great side effects
having code not duplicated is less error prone...
Now also repl getting LittleFS Configuration: which was missing
Have print that tell the Image Size: size and the Input Image Size (compacted):
Have new test that catching this issue (according beyonce rule)
Now that all creation of littlefs pass through _fs_from_args it will be easier for me to implement #150
Left issue
Now REPL succeed to mount and work with compacted image, but when changing the image (with put command for example) it works but the image is being partially compacted (depends on where littlefs decide to put the data). Need design decision here what is expected behaviour, anyway I think it is different task and this one should be merged as is.