Skip to content

Conversation

@lubosz
Copy link
Contributor

@lubosz lubosz commented Nov 8, 2025

While being a good practice to use during development, enabling -Werror by default for deployment and user builds is a guarantee for failed builds.

It is not feasible for development to keep up with all warnings in all compiler versions, especially ones from the future.

To avoid the build being broken, this patch sets -Werror being disabled as default behavior. This mimics the behavior of CMake being invoked directly without the build.sh script.

While a zero warning policy is good, and -Werror should also be enabled by default on the CI, it is a bad practice for deployment.

https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend/

Problem solved by the commit

Project fails building on newer compiler versions.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

Providing a saner default.

Risks (if any) associated the changes in the commit

The project will more likely build on newer compiler versions.

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

While being a good practice to use during development, enabling -Werror
by default for deployment and user builds is a guarantee for failed
builds.

It is not feasible for development to keep up with all warnings in all
compiler versions, especially ones from the future.

To avoid the build being broken, this patch sets -Werror being disabled
as default behavior. This mimics the behavior of CMake being invoked
directly without the build.sh script.

While a zero warning policy is good, and -Werror should also be enabled
by default on the CI, it is a bad practice for deployment.

https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend/
@gbuildx
Copy link
Collaborator

gbuildx commented Nov 8, 2025

lubosz - is not a collaborator
Can XRT admins please validate PR

@stsoe
Copy link
Collaborator

stsoe commented Nov 20, 2025

@lubosz build.sh is for development, developers are required to build with errors as warning. While porting to new compilers, developers can disable -Werror as needed. Why do we need to change this?

@lubosz
Copy link
Contributor Author

lubosz commented Nov 21, 2025

@stsoe Thank you for your response.

This is just a recommendation for the reasons listed above.

build.sh is for development

According to the documentation, build.sh is the primary way of building XRT.
https://xilinx.github.io/XRT/master/html/build.html#building-the-xrt-runtime

This means that everyone following this documentation, may it be developer, distributor or advanced user, will face -Werror if they use a compiler version more recent than the version the XRT developers used at the time of the code release.

It's just a frustrating experience for on-boarding new developers.

In the end this MR is just to stimulate thoughts, I did not expect it to be merged. Maybe pointing the fact that -Werror is enabled by default and how to disable it in the documentation is another valid way to improve this.

I did my duty and started my contribution history to this project by fixing a warning that was treated as error, a review would be very appreciated:
#9419

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.

3 participants