-
Notifications
You must be signed in to change notification settings - Fork 22
ci: treat warnings as errors #1062
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
base: develop
Are you sure you want to change the base?
Conversation
.github/workflows/ci.yml
Outdated
llvm-archive-filename: {{{ llvm-archive-basename }}}.{{{ llvm-archive-extension }}} | ||
llvm-sanitizer-config: {{#if (and (ne compiler 'clang') (ne compiler 'apple-clang'))}}{{else if ubsan}}Undefined{{else if asan}}Address{{else if msan}}MemoryWithOrigins{{/if}} | ||
common-flags-base: {{#if (ieq compiler 'clang')}}-gz=zstd {{/if}} | ||
common-flags-base: {{#if (ieq compiler 'clang')}}-gz=zstd {{/if}}{{#if (eq compiler 'msvc') }}/WX {{else}}-Werror {{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this PR simpler. I don't know if these flags are being passed to dependencies. But one thing we often do in boost libraries is just to enable warnings as errors directly in CMake when the cmake option for tests is enabled. Then we enable it for the main libraries and for the test executables. In any case, even if using another strategy, we shouldn't be checking for errors in lua. In fact, I think I'm removing this lua bundle in another commit in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the lua sources are compiled as if they were part of mrdocs itself, and it would be annoying to have to provide different flags to different source files within mrdocs.
If you are removing the lua bundle anyway, then we might as well wait for that.
But otherwise, what's happening to it, are we going to be using the system lua implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Lua integration has never been completely implemented. In any case, it should be an external dependency, like any other, for several reasons, and that's one of them. I don't know if that removing lua blocks this PR but the other PR I mentioned should be ready today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well wait for that
No description provided.