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

LittleFS: Update to v1.7.2 #11624

Merged
merged 4 commits into from Oct 23, 2019
Merged

LittleFS: Update to v1.7.2 #11624

merged 4 commits into from Oct 23, 2019

Conversation

@geky
Copy link
Member

geky commented Oct 3, 2019

Release notes

v1.7.2: https://github.com/ARMmbed/littlefs/releases/tag/v1.7.2
v1.7: https://github.com/ARMmbed/littlefs/releases/tag/v1.7.0

Note, the release notes for patches has improved since this release here.

Changes

7e110b4 Added automatic version prefixing to releases
7f7b733 Added scripts/prefix.py for automatically prefixing version numbers
22b0456 Add missing word (and reflow text)
ec4d8b6 Changed release script to generate drafts
c7894a6 Added a handful of links to related projects
1950758 Added 2GiB file size limit and EFBIG reporting
97d8d5e Fixed issue where a rename causes a split and pushes dir out of sync
0bb1f7a Modified release script to create notes only on minor releases
28d2d96 Fix -Wsign-compare error
cb62bf2 Fixed release script issue with fetching recent tags
646b1b5 Added -Wjump-misses-init and fixed uninitialized warnings
e5a6938 Fixed possible infinite loop in deorphan step
6ad544f If stats file doesn't exist lfs_emubd_create will fail.
3419284 Fixed issue with corruption due to different cache sizes

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-storage

geky added 4 commits Oct 3, 2019
…orage/filesystem/littlefs/littlefs'

git-subtree-dir: features/storage/filesystem/littlefs/littlefs
git-subtree-split: 510cd13
…510cd13..7e110b44c0

7e110b44c0 Added automatic version prefixing to releases
7f7b7332e3 Added scripts/prefix.py for automatically prefixing version numbers
d3a2cf48d4 Merge pull request #135 from johnlunney/patch-1
22b0456623 Add missing word (and reflow text)
ec4d8b6 Changed release script to generate drafts
c7894a6 Added a handful of links to related projects
1950758 Added 2GiB file size limit and EFBIG reporting
97d8d5e Fixed issue where a rename causes a split and pushes dir out of sync
0bb1f7a Modified release script to create notes only on minor releases
447d89c Merge pull request #109 from OTAkeys/pr/fix-sign-compare
28d2d96 Fix -Wsign-compare error
cb62bf2 Fixed release script issue with fetching recent tags
646b1b5 Added -Wjump-misses-init and fixed uninitialized warnings
1b7a155 Merge pull request #106 from conkerkh/master
e5a6938 Fixed possible infinite loop in deorphan step
6ad544f If stats file doesn't exist lfs_emubd_create will fail.
3419284 Fixed issue with corruption due to different cache sizes

git-subtree-dir: features/storage/filesystem/littlefs/littlefs
git-subtree-split: 7e110b44c0e796dc56e2fe86587762d685653029
@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 3, 2019

@adbridge, I'm working on a script that will create these PRs automatically. Does this format work for your release scripts? Feedback is welcome.

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Oct 3, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 3, 2019

@geky, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 3, 2019

@geky in the current patching scheme then those merge commits would cause a lot of problems with git cherry-pick or git am. However once we move over to the new releasing/branching scheme in January when we release from Master then this would work fine.

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 3, 2019

@adbridge, do the merges cause a problem for minor releases? I thought the cherry-picking only happens on patch releases.

Unfortunately those merges are necessary for subtrees to work from what I understand.

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 3, 2019

@geky no, only a problem for patch releases currently. Not a problem for Minor/Feature releases and as I said above, won't be a problem at all from January onwards :)

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 3, 2019

Sounds like for now I'll keep the auto-generated PRs on minor releases. Patches can be cherry-picked as needed until January : )

@bulislaw

This comment has been minimized.

Copy link
Member

bulislaw commented Oct 4, 2019

@jamesbeyond do we run storage test that use littlefs on PR? If not can we run nightly on this?

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

jamesbeyond commented Oct 4, 2019

@jamesbeyond do we run storage test that use littlefs on PR? If not can we run nightly on this?

These are not greentea test. I think it is run as part of Travis job bellow travis-ci/littlefs

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

jamesbeyond commented Oct 4, 2019

Oh, actually, there are equivalent version of Greentea test as well,
@geky could you update those test together as well?
https://github.com/ARMmbed/mbed-os/blob/master/features/storage/filesystem/littlefs/TESTS

@adbridge adbridge added needs: work and removed needs: review labels Oct 7, 2019
@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Oct 8, 2019

Why "needs: Work" label?

Git subtree merges have been OK with Nanostack team for years, should not be problem here.

Copy link
Contributor

jamesbeyond left a comment

@pilotak

This comment has been minimized.

Copy link
Contributor

pilotak commented Oct 11, 2019

@geky why update to v1.7.2 when there is v2.1.2 available?

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 11, 2019

Why "needs: Work" label?

Git subtree merges have been OK with Nanostack team for years, should not be problem here.

Firstly the needs work is due to the comments made by @jamesbeyond . Secondly subtree merges are not handled in the automated release script and we do not have the time to do all these manually.

Copy link
Member

0xc0170 left a comment

Changes appear to be fixes, what is here functionality change?

Please add Release notes section , see https://os.mbed.com/docs/mbed-os/v5.14/contributing/workflow.html#pull-request-template-functional-changes

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 15, 2019

Changes appear to be fixes, what is here functionality change?

I marked it as a functionality change to suggest it should be on a minor release, otherwise it would break the release script due to subtree merges.

Please add Release notes section

There is a release note section, but I moved it to the top since it seemed like the most important part of the PR body, does the order cause problems?

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 15, 2019

Oh, actually, there are equivalent version of Greentea test as well,
@geky could you update those test together as well?

@jamesbeyond, sorry I haven't been able to keep up with GitHub notifications.

The Greentea tests were based on the local littlefs tests, but they're not in sync. The local tests can manipulate the filesystem from a higher-level than is available in Greentea:
https://github.com/ARMmbed/mbed-os/blob/master/features/storage/filesystem/littlefs/littlefs/tests/test_orphan.sh#L20

I did actually have a script for converting the local tests -> Greentea tests, but it's broken for various reasons:
https://github.com/ARMmbed/mbed-os/tree/master/features/storage/filesystem/littlefs/TESTS/util

I get that it would be valuable to duplicate the tests into Greentea, but I can't right now. There's other tasks in littlefs that have my priority.

They are at least ran in qemu-arm in the littlefs repo:
https://github.com/ARMmbed/littlefs/blob/master/.travis.yml#L73-L86

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 15, 2019

@geky why update to v1.7.2 when there is v2.1.2 available?

Because v2 is a breaking change and Mbed OS users have existing devices with v1. There's a v1 -> v2 migration but it comes with a risk and requires testing effort.

There is https://github.com/armmbed/mbed-littlefs (sorry it's still pending an update), and I'm hoping to merge it into Mbed OS, but we'll still have v1 around.

At least the script to manage updates should work for both versions.

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 15, 2019

@geky we need this PR to be merged within the next 3 works so would you have time to update this by then? Or is there somebody else that could help ?

Alternatively could you add this :
https://github.com/ARMmbed/littlefs/blob/master/.travis.yml#L73-L86
to our travis so that it runs against all PRs ? Including this one ?

@jamesbeyond

This comment has been minimized.

Copy link
Contributor

jamesbeyond commented Oct 15, 2019

Hi @geky , totally understand. It's going to require engineer effort to get them in sync.

I can see there are no corresponding alloc and truncate Greentea test.
Maybe could you help with most straightforward one :
https://github.com/ARMmbed/mbed-os/blob/master/features/storage/filesystem/littlefs/TESTS/filesystem/seek/main.cpp#L538

int -> unsigned, that would help as well 😉

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 16, 2019

@jamesbeyond As this PR bringing fixes, tests update can happen separately or this is blocked on them?

@0xc0170 0xc0170 self-requested a review Oct 16, 2019
@geky

This comment has been minimized.

Copy link
Member Author

geky commented Oct 16, 2019

@adbridge, oh, the local tests are already running in Mbed OS's travis:
https://travis-ci.org/ARMmbed/mbed-os/jobs/593092561

Are you wanting the tests to run under qemu-arm in Thumb mode as well? There was some reason we didn't want qemu in CI, but if that's not an issue I can add it in.

@jamesbeyond, the problem is that the filesystem API's are different enough between the littlefs C api <-> Mbed OS C++, that translating between the two is non-trivial.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 18, 2019

@jamesbeyond Did @geky answer testing request? Do we need to do anything else for this PR?

Copy link
Contributor

jamesbeyond left a comment

I can see the Littlefs Test is running in the Travis.
As @geky confirm that the LittleFS c API differ from mbed-os API, I am OK for leave the greentea test as it

@adbridge adbridge added needs: CI and removed needs: work labels Oct 23, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 23, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 23, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Oct 23, 2019
@0xc0170 0xc0170 merged commit bb48dd3 into ARMmbed:master Oct 23, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8689 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@geky

This comment has been minimized.

Copy link
Member Author

geky commented Nov 18, 2019

If someone hypothetically wanted to cherry-pick these changes onto some sort of, I don't know, patch release. These commands should work:

git cherry-pick -m 1 55f961b931a32c941cdba3fd4771c57bd453563d --allow-empty
git cherry-pick -m 1 de8df75ec0b50c772629670534bf264aae96af48
@40Grit

This comment has been minimized.

Copy link

40Grit commented Nov 18, 2019

@AGlass0fMilk a wild @geky appeared ^^^

@geky

This comment has been minimized.

Copy link
Member Author

geky commented Nov 18, 2019

@40Grit, hello, was someone looking for me? Sorry I've not been on top of GitHub notifications for a while.

@loverdeg-ep

This comment has been minimized.

Copy link
Contributor

loverdeg-ep commented Nov 18, 2019

@AGlass0fMilk has something in your email inbox I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.