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

Extrapolate an interface for reading Zephyr nvs data. #9

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

pasinskim
Copy link
Member

In many place we are reading the length of the data, allocating memory, and then using allocated memory to read the actual data. Instead of repeating it in many places we can use an interface to deal with it and use when we need to access nvs stored data.

@pasinskim pasinskim force-pushed the main branch 3 times, most recently from 1b0b0f9 to 155efb0 Compare July 11, 2024 15:00
@danielskinstad
Copy link
Collaborator

fyi: To get the pipeline to pass, you can do something hacky like this: docker run -v path/to/mender-mcu:/mender-mcu -it ubuntu:20.04 and format your files with the clang-format in the container

@lluiscampos
Copy link
Collaborator

fyi: To get the pipeline to pass, you can do something hacky like this: docker run -v path/to/mender-mcu:/mender-mcu -it ubuntu:20.04 and format your files with the clang-format in the container

The OS project is going to accept my contribution (see joelguittet/mender-mcu-client#67) so I launched a similar one for our fork (see #10) with the intention of avoiding exactly that sort of hacky formatting 😅

Copy link
Collaborator

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Nice! 👏

@pasinskim
Copy link
Member Author

@danielskinstad @lluiscampos this is pretty weird. I run the clang-format locally and everything passes. Need to figure out how to run it locally exact same way as it is done in the CI.

@danielskinstad
Copy link
Collaborator

@danielskinstad @lluiscampos this is pretty weird. I run the clang-format locally and everything passes. Need to figure out how to run it locally exact same way as it is done in the CI.

I have this version

daniel@daniel-thinkpad:~/.../repos/mender-mcu$ clang-format --version
clang-format version 18.1.8

and use this script, which seems to work:

for source_file in `git ls-tree -r HEAD --name-only | grep -E '(.*\.c$|.*\.h$|.*\.cpp$|.*\.hpp$)' | grep -vFf .clang-format-ignore`
do
    lines=$(clang-format -i ${source_file} 2>&1 | wc -l)
done

@pasinskim
Copy link
Member Author

@danielskinstad for testing can you run your clang-format on my latest commit and see which places it complains about? Thanks.

@danielskinstad
Copy link
Collaborator

danielskinstad commented Jul 15, 2024

@danielskinstad for testing can you run your clang-format on my latest commit and see which places it complains about? Thanks.

Hmm, it is as you said: it passes locally. I even tried the clang-format from ubuntu:24.04 locally, and that also passed. Weird, tried running it manually from gitlab aswell, still fails.

EDIT:

I checked the wrong thing, I assumed it was ./.github/workflows/check_code_format.sh that failed, but it's ./.github/workflows/check_equivalence_tests.sh. This fails locally.

In many place we are reading the length of the data, allocating memory,
and then using allocated memory to read the actual data. Instead of
repeating it in many places we can use an interface to deal with it and
use when we need to access nvs stored data.

Changelog: Title

Ticket: MEN-7380

Signed-off-by: Marcin Pasinski <[email protected]>
@mender-test-bot
Copy link

mender-test-bot commented Jul 15, 2024

Merging these commits will result in the following changelog entries:

Changelogs

mender-mcu (main)

New changes in mender-mcu since main:

  • Extrapolate an interface for reading Zephyr nvs data.
    (MEN-7380)

@pasinskim
Copy link
Member Author

@danielskinstad thanks! I spend way too much time trying to figure out what is wrong with formatting and completely missed check_equivalence_tests.sh.

Copy link
Collaborator

@danielskinstad danielskinstad left a comment

Choose a reason for hiding this comment

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

Just fyi, if there is no change in functionality for the user, you don't need to include changelog. Usually used when it's a fix or a feat.
https://github.com/mendersoftware/mendertesting/blob/master/commitlint/grammar.md

@pasinskim pasinskim merged commit c2e2daa into mendersoftware:main Jul 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants