Skip to content

sys/log_*: modularize log into log_color and log_printfnoformat#18425

Merged
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
kfessel:p-modular-log
Oct 13, 2022
Merged

sys/log_*: modularize log into log_color and log_printfnoformat#18425
leandrolanzieri merged 3 commits intoRIOT-OS:masterfrom
kfessel:p-modular-log

Conversation

@kfessel
Copy link
Copy Markdown
Contributor

@kfessel kfessel commented Aug 9, 2022 •
edited
Loading

Contribution description

this transforms sys/log into two simple modules

this also makes log_write nonstatic for log_color
(saving a lot of space in builds) from #18403

Testing procedure

there are log tests all variants

Issues/PRs references

this arose from the discussion in #18403
this also solves #18379

@github-actions github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: timers Area: timer subsystems Area: tools Area: Supplementary tools labels Aug 9, 2022
@kfessel kfessel requested a review from miri64 August 9, 2022 12:44
@kfessel kfessel requested a review from maribu August 10, 2022 10:39
@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Aug 10, 2022
@kfessel kfessel changed the title sys/log: less special just modules sys/log: make log less special - just 2 modules Aug 10, 2022
@github-actions github-actions bot removed Area: tools Area: Supplementary tools Area: timers Area: timer subsystems labels Aug 10, 2022
@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Aug 10, 2022

i moved the doccheck changes to #18431 and added the Kconfig adaption

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 10, 2022
@kfessel kfessel mentioned this pull request Aug 11, 2022
Closed
miri64
miri64 reviewed Aug 11, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

log_% should pull in log as a dependency in sys/Makefile.dep, so it also works as long as we still use "legacy" dependencies.

@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Aug 11, 2022 •
edited
Loading

log_% should pull in log as a dependency in sys/Makefile.dep, so it also works as long as we still use "legacy" dependencies.

this already is the case in master as this was needed for the pseudomodules as well

https://github.com/RIOT-OS/RIOT/blob/master/sys/Makefile.dep#L399-L402

and unchaged in this PR

https://github.com/kfessel/RIOT/blob/039f1835dd9e002dce03ccd0f7cd8f9fa87feedf/sys/Makefile.dep#L399-L402

@kfessel kfessel requested a review from benpicco September 15, 2022 13:14
@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Sep 15, 2022

@maribu and @benpicco showed some interest i this PRs predecessors

@kfessel
sys/log: modularize log into log_color and log_printfnoformat
514325b 
 - log_color: make log_write nonstatic
 - log_printfnoformat
 - apply module split to Kconfig
@kfessel
sys/test_utils/print_stack_usage: include stdio if printf is used
bac70c5
@kfessel kfessel changed the title sys/log: make log less special - just 2 modules sys/log_*: modularize log into log_color and log_printfnoformat Oct 12, 2022
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 12, 2022 •
edited
Loading

Murdock results

✔️ PASSED

e89063e sys/log_color: guard from compiling for esp

Success Failures Total Runtime
1980 0 1980 06m:37s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

benpicco
benpicco approved these changes Oct 12, 2022
View reviewed changes
Copy link
Copy Markdown
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.

That's a sensible cleanup.

@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 13, 2022
@maribu
Copy link
Copy Markdown
Member

maribu commented Oct 13, 2022

Not having a copy of the log function in every object file is IMO a bugfix.

@leandrolanzieri I'd like to still have this. Please hit merge if you have no objections.

@leandrolanzieri leandrolanzieri merged commit fa96d46 into RIOT-OS:master Oct 13, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@kfessel kfessel mentioned this pull request Oct 17, 2022
Closed
@kfessel kfessel mentioned this pull request May 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@benpicco benpicco benpicco approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@bergzand bergzand Awaiting requested review from bergzand

@maribu maribu Awaiting requested review from maribu

@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

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: Kconfig Area: Kconfig integration Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

6 participants

@kfessel @riot-ci @maribu @miri64 @benpicco @leandrolanzieri