Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

mjson: Initial include of package #20129

Merged
merged 3 commits into from
Dec 18, 2023
Merged

mjson: Initial include of package #20129

merged 3 commits into from
Dec 18, 2023

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Dec 1, 2023

Contribution description

This PR adds a package for mjson

Testing procedure

A small test application is provided

Issues/PRs references

None

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: build system Area: Build system Area: pkg Area: External package ports Area: Kconfig Area: Kconfig integration labels Dec 1, 2023
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2023
aabadie
aabadie reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Maybe add an app.config.test to check Kconfig (and add Kconfig dependency modelling) ?

@riot-ci
Copy link

riot-ci commented Dec 1, 2023 •
edited
Loading

Murdock results

✔️ PASSED

18c1548 tests/pkg/mjson: Add BOARD_INSUFFICIENT_MEMORY list

Success Failures Total Runtime
8098 0 8098 10m:18s

Artifacts

@bergzand
Copy link
Member Author

bergzand commented Dec 1, 2023

Maybe add an app.config.test to check Kconfig (and add Kconfig dependency modelling) ?

Added an app.config.test file. Not sure if there is more Kconfig dependency to model here.

benpicco
benpicco reviewed Dec 3, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good, just please add a comment as to why MJSON_ENABLE_MERGE=0 is set.

Not sure if there is more Kconfig dependency to model here.

Since Kconfig is on it's way out, I don't think we need to worry too much about that.

pkg/mjson/mjson.mk
@@ -0,0 +1,5 @@
MODULE = mjson

CFLAGS += -DMJSON_ENABLE_MERGE=0
Copy link
Contributor

@benpicco benpicco Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please add a comment as to why this option is chosen - you can squash directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Squashed in a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually do provide it (well, newlib/picolibc does) but it's a bad idea to use it with our small stacks.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. On the mjson side an include for alloca is missing, that's why it didn't compile for me. I've rephrased the comment a bit to clarify

@bergzand
Copy link
Member Author

bergzand commented Dec 7, 2023

Since Kconfig is on it's way out, I don't think we need to worry too much about that.

Are you suggesting to leave out the Kconfig integration here? Or just the app.config.test file?

@benpicco
Copy link
Contributor

benpicco commented Dec 7, 2023

I'm just saying don't put too much work into Kconfig when we are going to delete it anyway by the end of the month.

benpicco
benpicco reviewed Dec 7, 2023
View reviewed changes
@benpicco benpicco enabled auto-merge December 7, 2023 16:33
@benpicco
Copy link
Contributor

benpicco commented Dec 8, 2023

CI is not yet happy 😕

@MrKevinWeiss
Copy link
Contributor

Looks like you either need to add an rsource in the pkg/Kconfig to bring in the module or remove kconfig and the app.config.test to disable the test.

@benpicco benpicco added this pull request to the merge queue Dec 11, 2023
@bergzand
Copy link
Member Author

Looks like you either need to add an rsource in the pkg/Kconfig to bring in the module

Thanks, completely missed that bit!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2023
@benpicco benpicco added this pull request to the merge queue Dec 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 14, 2023
@MrKevinWeiss MrKevinWeiss added this pull request to the merge queue Dec 18, 2023
Merged via the queue into RIOT-OS:master with commit 5ba18df Dec 18, 2023
@bergzand bergzand deleted the pr/mjson/initial branch December 18, 2023 16:11
@bergzand
Copy link
Member Author

Thanks for adding the final bits @MrKevinWeiss ❤️

@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@aabadie aabadie aabadie left review comments

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@smlng smlng Awaiting requested review from smlng

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
5 participants
@bergzand @riot-ci @benpicco @MrKevinWeiss @aabadie