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: Add a kconfig to override littlefs version #14903

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Nov 22, 2024

Summary

Hopefully, It's more user-friendly than the current method, which is just an overridable make variable. It would also help to make some features (and their associated local patches) conditional on littlefs version in future.

Also, update CMakeLists.txt to use the same littlefs version as Makefile.

Impact

this changes littlefs version for cmake users.

Testing

esp32s3-devkit:toywasm

Hopefully, It's more user-friendly than the current method,
which is just an overridable make variable. It would also help
to make some features (and their associated local patches)
conditional on littlefs version in future.

Also, update CMakeLists.txt to use the same littlefs version as Makefile.
@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Nov 22, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 22, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details in several sections.

Here's a breakdown of the missing information:

Summary:

  • Missing: The summary is vague. What exactly is the change? Is a configuration option being added? Is the littlefs library itself being updated? How is it more user-friendly? Be specific! Mention the previous method (the overridable make variable) by name.
  • Missing: What functional part of the code is being changed? (e.g., Configuration system, littlefs integration, build system)
  • Missing: A clear description of how the change works. What is the new mechanism? Walkthrough a concrete example.
  • Missing: Links to related NuttX issues or NuttX Apps issues/PRs if applicable.

Impact:

  • While it mentions a change for CMake users, it lacks detail. How will they need to adapt? Will any configuration options change? Will existing user configurations break?
  • Missing: Clear YES/NO answers for all impact categories, followed by descriptions where applicable. Even if the answer is NO, explicitly state it. For example:
    • Impact on build: YES (CMake build system updated)
    • Impact on hardware: NO
    • Impact on documentation: YES (Documentation needs to be updated to reflect the new configuration method)
    • Impact on security: NO
    • Impact on compatibility: Potentially YES (If the littlefs version change introduces API breaks, this needs to be addressed)
  • The current answer "this changes littlefs version for cmake users" belongs in the description following a "YES" for "Impact on User".

Testing:

  • Insufficient: esp32s3-devkit:toywasm is not enough information. What OS was the build host? What compiler version? What specific tests were run? Were any other targets tested?
  • Missing: "Testing logs before change" and "Testing logs after change" sections are empty. These are crucial for demonstrating that the change works as intended and doesn't introduce regressions. Include relevant log output showing the littlefs version before and after the change, and any functional tests demonstrating the feature works.

In short, the PR needs significantly more detail and concrete information to meet the NuttX requirements. Focus on clarity, specificity, and providing evidence of testing.

@yamt
Copy link
Contributor Author

yamt commented Nov 22, 2024

msys2 job failure looks unrelated


   perl-HTTP-Cookies-6.11-1-any downloading...
   perl-Error-0.17029-2-any downloading...
  error: failed retrieving file 'texinfo-7.1.1-1-x86_64.pkg.tar.zst' from mirror.msys2.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
  error: failed retrieving file 'glib2-2.82.2-1-x86_64.pkg.tar.zst.sig' from ftp.osuosl.org : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds
  warning: failed to retrieve some files
  error: failed to commit transaction (unexpected error)
  Errors occurred, no packages were upgraded.
  Error: The process 'C:\Windows\system32\cmd.exe' failed with exit code 1

@simbit18
Copy link
Contributor

This also is unrelated

====================================================================================
Cmake in present: nrf52832-dk/mcuboot_loader,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Configuration/Tool: nrf52832-dk/mcuboot_loader,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2024-11-22 08:13:34
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Select HOST_LINUX=y
CMake Error at /github/workspace/sources/nuttx/build/_deps/mcuboot-subbuild/mcuboot-populate-prefix/src/mcuboot-populate-stamp/download-mcuboot-populate.cmake:170 (message):
  Each download failed!

    error: downloading 'https://github.com/mcu-tools/mcuboot/archive/414ac87cfd8d9cedeb781f812ad6f5072e6d8a39.tar.gz' failed
          status_code: 28
          status_string: "Timeout was reached"
          log:
          --- LOG BEGIN ---
            Trying 140.82.114.3:[443](https://github.com/apache/nuttx/actions/runs/11968574208/job/33367692699#step:7:444)...

@xiaoxiang781216 xiaoxiang781216 merged commit 72a8764 into apache:master Nov 22, 2024
27 checks passed
yamt added a commit to yamt/incubator-nuttx that referenced this pull request Dec 16, 2024
Note that $(CONFIG_FS_LITTLEFS_VERSION).tar.gz is expanded to
eg. "v2.5.1".tar.gz. The extra quotes break make's file existence
checks.

Fixes: apache#15148

The regression was caused by apache#14903
xiaoxiang781216 pushed a commit that referenced this pull request Dec 16, 2024
Note that $(CONFIG_FS_LITTLEFS_VERSION).tar.gz is expanded to
eg. "v2.5.1".tar.gz. The extra quotes break make's file existence
checks.

Fixes: #15148

The regression was caused by #14903
jerpelea pushed a commit to jerpelea/nuttx that referenced this pull request Dec 16, 2024
Note that $(CONFIG_FS_LITTLEFS_VERSION).tar.gz is expanded to
eg. "v2.5.1".tar.gz. The extra quotes break make's file existence
checks.

Fixes: apache#15148

The regression was caused by apache#14903
xiaoxiang781216 pushed a commit that referenced this pull request Dec 17, 2024
Note that $(CONFIG_FS_LITTLEFS_VERSION).tar.gz is expanded to
eg. "v2.5.1".tar.gz. The extra quotes break make's file existence
checks.

Fixes: #15148

The regression was caused by #14903
linguini1 pushed a commit to CarletonURocketry/nuttx that referenced this pull request Jan 15, 2025
Note that $(CONFIG_FS_LITTLEFS_VERSION).tar.gz is expanded to
eg. "v2.5.1".tar.gz. The extra quotes break make's file existence
checks.

Fixes: apache#15148

The regression was caused by apache#14903
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 Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants