Skip to content

Add -Werror to CXXFLAGS for dev CI job - #1501

Open
Enchufa2 wants to merge 2 commits into
masterfrom
feature/Werror
Open

Add -Werror to CXXFLAGS for dev CI job#1501
Enchufa2 wants to merge 2 commits into
masterfrom
feature/Werror

Conversation

@Enchufa2

@Enchufa2 Enchufa2 commented Sep 1, 2026

Copy link
Copy Markdown
Member

As a first step towards #1497, this simply converts warnings into errors for the checks with R devel, which I thought it was the most appropriate one. All checks still pass because I didn't activate any extra warnings yet.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

@eddelbuettel eddelbuettel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A good start

@Enchufa2

Enchufa2 commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

The only problem with this setup is that, when tests fail due to warnings turned into compilation errors, these issues are not shown in the runner logs, but in R's own install or check logs. So we could add an additional step after the checks that, when these fail, either

  • cats the relevant output; or
  • uploads the Rcpp.Rcheck folder as an artifact for further inspection.

I personally like the second option.

@eddelbuettel

Copy link
Copy Markdown
Member

Had we discussed this before you altered an existing file 😆 I might have convinced you to start with a new one 😆. If we start from the plain r-ci template r-ci.yaml you note that it already has a part to dump logs in case of failure():

https://github.com/eddelbuettel/r-ci/blob/4316c2c3cd96eb457ab2ca4e04abe52146c9e61c/docs/r-ci.yaml#L36-L38

Adding an artifact is good, and easy to add in a one-off file.

But as always there are more than one way to skin a cat and you are in the driver's seat, so other ideas welcome too.

@Enchufa2

Enchufa2 commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

I thought it would save some CPU cycles and therefore some water if we reused the ci-dev run already in place, but happy to add a new yaml if you think is cleaner. :)

Same applies to cat vs artifact. I personally find the artifact useful, but I'm fine with dumping the logs if you prefer.

@eddelbuettel

Copy link
Copy Markdown
Member

I am open to either approach. What I want to avoid, mostly, is for this to inflict ❌ on routine PRs and updates. If we can avoid that, good. If we cannot, I would think an opt-in is cleaner. Let's see how it goes.

@Enchufa2

Enchufa2 commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

Ok, let's go for the cleaner approach then. :) Give me a sec.

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.

2 participants