-
-
Notifications
You must be signed in to change notification settings - Fork 994
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
Suppress warnings in third-party code #7319
Conversation
LGTM; I don't have the time needed to test it ATM, but the idea looks very good (and is basically solving a blocker for me :), so my personal opinion on this is "merge ASAP" :D |
@DomClark BTW, do we have some code/workflow guidelines for LMMS repo? I guess so, but I couldn't find them. I'm asking wr.t. the commit naming for PRs - also, are we squashing/rebasing? AFAIR, the commits from my PRs were merged verbatim, so that would raise the question whether e.g. a squash/rebase would be needed here, to get rid of the "attempt"/"try" etc. WIP commits? |
Dw about it, we'll squash it finally |
@Rossmaxx out of curiosity, doesn't it stay in the squashed commit message though as bullet points? |
No one looks at the bullet points, or the person who merges might delete the cleanup points from the message. |
be wary of using the existential or universal quantifier :D |
@DomClark I've rebased on your branch and updated the relevant stuff, but on GCC 13 https://github.com/LMMS/lmms/actions/runs/9529541161/job/26268401821?pr=7283#step:9:2777 is still there (it happens entirely in |
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.
/plugins/GigPlayer/CMakeLists.txt should probably also be updated to leverage SYSTEM
That's a good point. I'd only made changes relating to the third-party code that we submodule, assuming that system-provided headers would already be considered as such. While this is true for some libraries that we consume as imported targets (imported targets are implicitly flagged as |
As Rossmaxx says, we generally use GitHub's "squash and merge" feature, but there are occasional cases when we don't. I tend to remove the bullet points that GitHub automatically puts into the commit message box, unless they happen to add useful information (which is rarely the case). However, we don't have an official procedure, and some people leave the default message unedited. As an exception, co-author lines we always leave untouched, to make sure people remain credited As for commit/PR naming, we tend to use what is industry-standard, i.e. a short title in the imperative mood. This is even less enforceable than the merge procedure, of course, as anyone can open a pull request. I usually avoid editing PR titles, so as not to appear rude to people who may not speak English fluently, but I'd be lying if I said I weren't ever so slightly bothered by titles such as "adds feature" or "added feature" instead of "add feature". When merging a pull request, GitHub automatically adds its number in brackets to the end of the commit message (although this can be edited), so it's worth trying to make PR titles extra short, as they will be longer once merged. Other peeves of mine are things like "(rework)" at the end of PR titles - this is meaningless in commit history, as the original PR won't have been merged - and issue numbers, which can be confusing when the PR number is added too at merge time. Additionally, an issue number is not a substitute for a description of what was fixed, as it is meaningless outside of GitHub. In short, there are no hard-and-fast rules, but think about what the message will look like when merged, and whether it will be meaningful to someone who is looking at the commit locally, rather than on GitHub. If you ever want a particular description to be included when a PR is merged, feel free to say. |
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.
tested on #7283 and works like a charm with GCC 13 (don't mind the MinGW 32-bit failing, it's completely unrelated to this one) - can't wait to see it merged!
Checking the configure output... Not sure if we want to handle any of these here or not. We have quite a few of these:
I think this is new:
Build output had this show up now:
|
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.
LGTM, easy to confirm changes work with CI since this is build system
See #7322 for the CMake warnings. This PR is intended to address C/C++ warnings. |
This should improve compatibility with newer compiler versions, and allow us to turn up the warning level for our own code. I have made no attempt to fix any existing warnings in this PR, and haven't removed any warning suppression flags, except those that applied only to third-party code or were duplicated.
The main technique here is to ensure all third-party code is built in separate targets from first-party code, and to set the
SYSTEM
property on those targets. This property will cause include directories associated with these targets to be passed to the compiler as system include directories, suppressing warnings in the headers they contain (automatically in the case of GCC and Clang, and with the addition of/external:W0
in the case of MSVC). To suppress warnings when building third-party source files, we check theSYSTEM
property with a generator expression, which determines which set of warning flags to pass.The biggest difficulty in separating third-party code into its own targets was getting everything to link properly afterwards; this is the source of most of the complexity in this PR. Object libraries cannot be linked transitively (only the usage requirements propagate, not the objects themselves), whereas static libraries publicly targeted by a shared library will also be included when linking dependents of that shared library (which is undesirable when the shared library is meant to export the symbols from the static library). To address this, I have written a helper module to support a new static linking scope. CMake has two scopes when setting attributes on a target: the private scope applies only to the target in question, whereas the interface scope applies to all of the target's transitive dependents, at least until it reaches a private scope (the public scope is simply both of these scopes together). The static scope applies only to statically-linked transitive dependents, i.e. those within the same dynamic library or executable - it will not extend to dependents that link dynamically to the target in question.
The core now needs some compile definitions from libraries it depends on. We used to have the
lmms
target depend on all the libraries required by the core, and manually copy the include directories over tolmmsobjs
. Rather than extend this to copy compile definitions too, I have taken advantage of the newer CMake version available to us and hadlmmsobjs
itself depend on those libraries, withlmms
inheriting the requirements through a public dependency onlmmsobjs
.I have considered the CAPS plugin suite to be third-party, despite it being vendored into our source tree. There is an effort to submodule it in #4027, and we don't maintain the vendored copy much anyway.
I have removed the external weakjack library that was installed into
plugins/optional
(but not the copy built into the core), as I believe it exists by mistake. It was added tostable-1.2
in #3958, and tomaster
in #3970. It was then removed fromstable-1.2
in #4186, but not removed frommaster
, even when merging fromstable-1.2
. The comment at #4094 (comment) suggests that it is no longer required, and there are no references to it left that I can find.