Skip to content

Conversation

@dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Nov 6, 2024

Summary

This fixes a regression in db59e55: prior to that commit mpremote supported trailing slashes on the destination of a normal (non-recursive) copy.

Add back support for that, with the semantics that a trailing slash requires the destination to be an existing directory.

Testing

A test has been added to the mpremote tests (also a test for force copy, in a separate commit). As part of getting the test working, needed to flush stdout before printing error messages to stderr.

The mpremote tests were then run on a PYBD_SF2. They passed.

@dpgeorge dpgeorge added the tools Relates to tools/ directory in source, or other tooling label Nov 6, 2024
@dpgeorge
Copy link
Member Author

dpgeorge commented Nov 6, 2024

Note: this is related to #2466 and #2929: filesystems on bare-metal ports don't like trailing slashes in their filenames, even if it refers to a directory. So that's why trailing slashes need to be stripped before passing them down to the target board (also doing the strip in mpremote allows enforcing that anything ending in a slash is indeed a directory).

@codecov
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (6902362) to head (4fd5b72).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #16164   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21345    21345           
=======================================
  Hits        21041    21041           
  Misses        304      304           

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

@github-actions
Copy link

github-actions bot commented Nov 6, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@dpgeorge
Copy link
Member Author

dpgeorge commented Nov 6, 2024

This would also go in a v1.24.1 patch release.

@dpgeorge dpgeorge requested a review from projectgus November 7, 2024 01:52
@dpgeorge dpgeorge added this to the release-1.24.1 milestone Nov 12, 2024
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

LGTM!

mpremote error messages now go to stderr, so make sure stdout is flushed
before printing them.

Also update the test runner to capture error messages.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
This fixes a regression in db59e55: prior
to that commit `mpremote` supported trailing slashes on the destination of
a normal (non-recursive) copy.

Add back support for that, with the semantics that a trailing slash
requires the destination to be an existing directory.

Also add a test for this.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the tools-mpremote-fix-dest-trailing-slash branch from 0ad35d1 to 4fd5b72 Compare November 13, 2024 00:52
@dpgeorge
Copy link
Member Author

For some reason the windows mingw dev CI is failing, but that's also happening on master so it's nothing to do with this PR. That needs separate investigation.

@dpgeorge dpgeorge merged commit 4fd5b72 into micropython:master Nov 13, 2024
63 of 65 checks passed
@dpgeorge dpgeorge deleted the tools-mpremote-fix-dest-trailing-slash branch November 13, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools Relates to tools/ directory in source, or other tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants