Skip to content

Conversation

@maribu
Copy link
Member

@maribu maribu commented Aug 19, 2025

Contribution description

This tests makes little sense to have for a number of reasons:

  1. One should not use iprintf() for a number of reasons:
    1. It is non-standard and using it over printf() makes the code less portable (e.g. it cannot be used on AVR)
    2. The idea of adding a leaner variant of printf() in addition to the larger one is bogus, as apps will end up using both resulting in a larger firmware instead of a smaller
    3. RIOT's build system already has the printf_float module to control whether formatting of floating point numbers should be suppered. This mechanism will actually result in smaller builds, if floating point support is not needed, as it prevents two variants of printf to be linked in.
  2. The test checks some implementation details (e.g. whether the address of two functions is identical), rather than correct behavior of the implementation. This is completely bogus.

Testing procedure

Happy CI

Issues/PRs references

#9599 introduced this test.

As can be seen in the PR, the point of the test is to check whether using nano.specs from newlib via the module newlib_nano has the intended effect. But since it is up to whoever is building / packaging newlib to decide whether to provide a nano.specs at all and how the nano.specs variant is configured, this way of testing does not appear to be super valid.

This tests makes little sense to have for a number of reasons:

1. One should not use `iprintf()` for a number of reasons:
    1. It is non-standard and using it over `printf()` makes the code
       less portable (e.g. it cannot be used on AVR)
    2. The idea of adding a leaner variant of `printf()` in addition to
       the larger one is bogus, as apps will end up using both resulting
       in a *larger* firmware instead of a smaller
    3. RIOT's build system already has the `printf_float` module to
       control whether formatting of floating point numbers should be
       suppered. This mechanism will actually result in smaller builds,
       if floating point support is not needed, as it prevents two
       variants of printf to be linked in.
2. The test checks some implementation details (e.g. whether the
   address of two functions is identical), rather than correct behavior
   of the implementation. This is completely bogus.
@maribu maribu added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2025
@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework and removed Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2025
@crasbe
Copy link
Contributor

crasbe commented Aug 19, 2025

Was that test ever executed by our CI system? I don't think there is a test configuration which would set USEMODULE += newlib, which would be a precondition for this test to compile & run 🤔

@crasbe crasbe added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2025
@maribu
Copy link
Member Author

maribu commented Aug 19, 2025

The boards that use newlib by default have a newlib feature, which then is mirrored as a module, so that C code can check for its usage. (For some reason we have defines to check against for modules, but not for features.)

So this should indeed be executed. The fact that this fails in a PR that makes mpaland-printf the default for most newlib boards also supports that this is indeed used.

Copy link
Contributor

@Enoch247 Enoch247 left a comment

Choose a reason for hiding this comment

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

I concur with your assessment. As this test is broken when replacing newlib's printf implementation by pkg/mpaland-printf, I think we can drop this test since it is a bit fragile anyway.

@Enoch247 Enoch247 enabled auto-merge August 19, 2025 13:14
@Enoch247 Enoch247 disabled auto-merge August 19, 2025 13:14
@riot-ci
Copy link

riot-ci commented Aug 19, 2025

Murdock results

✔️ PASSED

7410255 tests/sys/libc_newlib: drop test

Success Failures Total Runtime
10538 0 10538 16m:58s

Artifacts

@Enoch247 Enoch247 added this pull request to the merge queue Aug 19, 2025
Merged via the queue into RIOT-OS:master with commit a23e100 Aug 19, 2025
30 checks passed
@benpicco benpicco added this to the Release 2025.10 milestone Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation 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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants