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

makefiles: No --fatal-warnings for Cortex-M linker #18843

Closed
wants to merge 1 commit into from

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Nov 4, 2022

Contribution description

The linker flag --fatal-warnings was added in #3120 on the premise that linker warnings result in broken binaries.

Issue #18522 has more details on a concrete issue, but the present PR doesn't resolve that issue's root cause -- the issue just serves to illustrate that there are linker warnings that are really just that (warnings), and that the original premise was flawed and introduces failures where there needn't be any.

Testing procedure

  • On Debian sid, this now works:
$ make -C examples/hello-world TOOLCHAIN=clang BOARD=microbit-v2

Issues/PRs references

Reverts-Part-Of: #3120
Contributes-To: #18522

Original round of reviewers are the original contributor of that flag, and the last mainitainers who touched RIOT's linker scripts.

@chrysn chrysn added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 4, 2022
@github-actions github-actions bot added the Platform: ARM Platform: This PR/issue effects ARM-based platforms label Nov 4, 2022
@riot-ci
Copy link

riot-ci commented Nov 4, 2022

Murdock results

✔️ PASSED

c322e07 makefiles: No --fatal-warnings for Cortex-M linker

Success Failures Total Runtime
2000 0 2000 06m:24s

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
Copy link
Contributor

benpicco commented Nov 7, 2022

Do we still need this with #18845?

@chrysn
Copy link
Member Author

chrysn commented Nov 7, 2022 via email

@maribu
Copy link
Member

maribu commented Nov 15, 2022

I'm not convinced. Humans get easily trained to ignore warnings, if they pop up often.

I think we, the RIOT core developers, should decided on whether to react on a warning or to dismiss it and not train application developers to ignore warnings.

@chrysn
Copy link
Member Author

chrysn commented Nov 15, 2022

Generally, yes, warnings need to be used carefully. But what happens in #18893 (and I might have done as well if #18845 had not been provided) is that concrete warnings are explicitly silenced until they are properly fixed. But once the warning is silent, nobody bats an eye any more, and all seems fine until something more changes (eg. the "this is deprecated and will be an error in the next release" triggers), and then things are broken just again, and possibly more subtly, because the warning kept being ignored.

We don't have build options that distinguish "a RIOT core developer is building it, show them warnings" and "hide warnings the devs deemed safe" (but are they really?), and I'm glad of that, because RIOT is a community project, and a drive-by contributor might have the right context none of the regulars currently has. Both regulars and occasional users can be prone to ignoring warnings, but things can break either way, and if there are warnings to be read at least one stands a chance of finding them in the logs.

@Teufelchen1
Copy link
Contributor

Whats the conclusion here?

@chrysn
Copy link
Member Author

chrysn commented Mar 19, 2024

I think the conclusion is just to be more careful when silencing warnings that are otherwise fatal (because that can hide real problems), and otherwise leave things as is here, and tolerate that at some point things may break again due to a tooling upgrade and we have to fix them in some way (eg. by silencing the warning for a short fix and then keeping a very active issue to fix it properly soonish.)

@chrysn chrysn closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants