Skip to content
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

fs/littlefs: stop applying local patches for unknown versions of littlefs #14956

Merged
merged 6 commits into from
Nov 26, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 26, 2024

Summary

fs/littlefs: stop applying local patches for unknown versions of littlefs

Because they are not expected to apply cleanly.
In that case, disable all features which require the local patches.

Note: lfs_util.patch is actually necessary regardless of
the littlefs version. It should be ok to drop it for the simplest
flat memory model configurations with a single heap though. I plan
to fix it once littlefs 2.9.4 is released.

Impact

littlefs

Testing

Tested with esp32s3-devkitc:toywasm + CONFIG_FS_LITTLEFS_VERSION="v2.9.3".

Build-tested linum-stm32h753bi/littlefs with cmake on ubuntu.

@github-actions github-actions bot added Area: File System File System issues Board: arm Board: xtensa Size: M The size of the change in this PR is medium labels Nov 26, 2024
…lefs

Because they are not expected to apply cleanly.
In that case, disable all features which require the local patches.

Note: lfs_util.patch is actually necessary regardless of
the littlefs version. It should be ok to drop it for the simplest
flat memory model configurations with a single heap though. I plan
to fix it once littlefs 2.9.4 is released.

Tested with esp32s3-devkitc:toywasm + CONFIG_FS_LITTLEFS_VERSION="v2.9.3".
I chose this config just because it seems to support cmake-based build.
I chose this config because I have a hardware and I occasionally
use littlefs on its flash.
@nuttxpr
Copy link

nuttxpr commented Nov 26, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR appears to mostly meet the NuttX requirements, but is missing some key information.

Strengths:

  • Summary provides a reasonable explanation of the why, what, and how. The reference to a future plan to address lfs_util.patch is good context.
  • Testing identifies the specific targets used for verification. Mentioning build testing on a different platform is also helpful.

Weaknesses:

  • Impact is severely lacking. The single word "littlefs" doesn't explain the impact to the user, build, hardware, documentation, security, or compatibility. Each of these needs to be addressed with a "YES/NO" and a description if "YES". For example:
    • Impact on user: YES. Users who were relying on features enabled by the local patches (which are now disabled for unknown littlefs versions) will experience a loss of functionality. These features should be listed.
    • Impact on build: Potentially YES. If the local patches were affecting the build process, then removing them might change it. This needs clarification.
    • Impact on hardware: Likely NO, but needs explicit confirmation.
    • Impact on documentation: Possibly YES. If the disabled features require documentation updates, this should be mentioned.
    • Impact on security: Needs explicit confirmation with YES/NO and explanation. Removing patches could have unintended security consequences.
    • Impact on compatibility: Likely YES due to the disabling of features. This needs to be explained in detail. Which versions are compatible now? Which are not?
  • Testing is missing the required "Testing logs before change" and "Testing logs after change" sections. While mentioning the test targets is good, the actual logs demonstrating the change in behavior are crucial for review.

Recommendation:

The PR author needs to significantly expand the Impact section with specific YES/NO answers and detailed explanations. They also must include the testing logs before and after the change to meet the NuttX requirements fully.

@xiaoxiang781216 xiaoxiang781216 merged commit 925573d into apache:master Nov 26, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Board: arm Board: xtensa Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants