Fix build with Clang #3208

jasp00 · 2017-01-04T19:00:23Z

Closes #3073.
Several issues were present, most of them because of third-party projects. System header gig.h may need to be overridden. This request adds a Clang build to Travis CI.

tresf · 2017-01-04T19:34:36Z

.travis/osx..script.sh

 fi

-cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWANT_QT5=$QT5 -DUSE_WERROR=OFF ..
+cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWANT_QT5=$QT5 -DUSE_WERROR=ON ..


We can simply remove this DUSE_WERROR now, right?

The default is off.

@jasp00 thanks I didn't realize this was our own flag. This was added and only ever enabled for Mac travis builds. My guess is @lukas-w did this just to get Travis to pass for the time being. If you've fixed all the clang warnings, is it needed anymore? 9cc1a59

To be more specific, couldn't we default it for DCMAKE_BUILD_TYPE=Debug like we do for stripping binaries, or is this flag a useful one to have around regardless?

USE_WERROR is not strictly necessary because you can use CFLAGS=-Wno-error and CXXFLAGS=-Wno-error. USE_WERROR should be dropped if Clang warnings go away.

tresf · 2017-01-05T03:41:48Z

.travis/linux..install.sh


 sudo apt-get install -y $PACKAGES
+
+# Fix header


@jasp00 should this be a cmake task so that it doesn't show up for non-travis builds too?

Non-Travis builds may not have sudo permission. We could add a task to warn about it and maybe fix it, but this is an issue that libgig upstream has dealt with and distros should handle their versions.

Non-Travis builds may not have sudo permission.

Can't we leverage a cmake task which places the custom header in user-space? Even if it's fixed upstream it will take a while for it to make it to the various -dev channels so it seems like we could hit two birds with one stone and it's not the only header we write at configure/compile time.

Can't we leverage a cmake task which places the custom header in user-space?

Yes, we can.

tresf · 2017-01-05T03:58:21Z

plugins/LadspaEffect/swh/analogue_osc_1416.c

 	float otm2 = plugin_data->otm2;
 	unsigned int rnda = plugin_data->rnda;
 	unsigned int rndb = plugin_data->rndb;
-	blo_h_tables * tables = plugin_data->tables;


Once these swh issues are merged, we should issue a PR against https://github.com/swh/ladspa

jasp00 · 2017-01-05T04:34:51Z

Request for upstream at swh/ladspa#44.

tresf · 2017-01-05T21:54:33Z

@jasp00 I see you added Clang + Qt5. I vote we make that the only Clang compiled build, since we're trying to move away from Qt4 anyway. Are there advantages to keeping Clang + Qt4 that you can think of?

jasp00 · 2017-01-05T22:26:57Z

Are there advantages to keeping Clang + Qt4 that you can think of?

We can easily enable this combination if the advantage arises.

jasp00 · 2017-01-24T11:09:00Z

Do you mind if I merge this one? It means an extra build for pull requests.

tresf · 2017-01-24T12:52:13Z

@jasp00 yes however can you explain the severity of items like re-writing headers on build, notably qvector?

jasp00 · 2017-01-24T13:31:41Z

can you explain the severity of items like re-writing headers on build, notably qvector?

The changes are harmless by themselves. The problem I see now is if a user does these steps:

Build in a broken environment.
Upgrade the environment.
Rebuild.

I will add a parameter to enable the rewriting.

tresf · 2017-01-24T13:47:38Z

I will add a parameter to enable the rewriting.

I meant, what breaks in the build if we don't rewrite those headers? Is it only so that we can have stricter compiler warning settings?

jasp00 · 2017-01-24T14:21:29Z

Is it only so that we can have stricter compiler warning settings?

It may be so. The errors could actually be warnings. I have not built with laxer settings.

tresf · 2017-01-24T14:29:21Z

I have not built with laxer settings.

I fit passes with laxer settings, can we consider removing it? What is the lesser of two evils here?

jasp00 · 2017-01-24T14:47:25Z

What is the lesser of two evils here?

Stricter settings are the best bet. Do not worry about rewriting, it is only meant for Travis CI. Users will not enable this option, unless they want to build with Clang, without warnings, and bad system headers.

lukas-w · 2017-01-24T16:15:49Z

How about just using Qt 5.7.1 which includes the fix? Homebrew seems to provide 5.7.1.

tresf · 2017-01-24T16:29:46Z

How about just using Qt 5.7.1 which includes the fix? Homebrew seems to provide 5.7.1.

That works. Since Qt5.7 still breaks macdeployqt we should probably merge in tresf@f24f3d9 shortly afterward so that packaging isn't adversely affected.

jasp00 · 2017-01-24T17:21:59Z

How about just using Qt 5.7.1 which includes the fix?

Linux build runs on Trusty, so an older Qt 5 should be supported anyway.

tresf · 2017-01-27T03:49:27Z

It may be so. The errors could actually be warnings. I have not built with laxer settings.

Linux build runs on Trusty, so an older Qt 5 should be supported anyway.

Sure but if the problem has been fixed upstream, isn't this overkill?

jasp00 · 2017-01-27T11:45:32Z

Sure but if the problem has been fixed upstream, isn't this overkill?

No. Trusty is not fixed. Some users build on Trusty. Travis builds on Trusty. As you can see, the builds are working. You need to configure with -DFIX_SYSTEM_HEADERS=ON to enable the rewriting, so it is controlled. Besides, /usr/include/gig.h needs to be fixed too, the fix being in experimental packages on some distros.

tresf · 2017-01-27T14:53:51Z

needs to be fixed

Mac builds with Clang on each release. I feel your use of the word "need" is unwarranted. If we need to rewrite headers, great. If you are just trying to avoid compilation warnings, I strongly feel this is overkill.

lukas-w · 2017-01-27T15:39:03Z

I agree with Tres. Another option to easily silence the warnings is just telling Clang not to show any warnings for these files by registering them as system headers. Adding a command line argument like --system-header-prefix=/usr/ should silence them.

tresf · 2017-01-27T16:30:47Z

--system-header-prefix=/usr should silence them.

Thanks for sharing. FYI, we'll need to be careful to make this version dependant due a Clang 3.4 bug -isystem-prefix vs. --system-header-prefix (Edit: and yes, I believe we're affected when we build from macOS 10.8 ~= XCode 5.1.1 ~= Clang 3.4):

http://stackoverflow.com/a/23342233/3196753

jasp00 · 2017-01-27T19:34:24Z

@tresf, you have asked what is worse, whether to build with laxer settings or to rewrite system headers. I have answered, stricter settings are better. Warnings in one system might become errors in another. Warnings are solved by fixing them, not by hiding them.

You have asked for a header rewrite. I have given you that and it works. Could you tell me what is actually wrong with fixing system headers? Why a bad header is better than a good one?

tresf · 2017-01-27T20:03:23Z

The advantage of keeping headers as-is is is simplicity, which the project needs more of.

I feel that is easier to uncomment two lines of code prefixed with # TODO: Remove once Travis ships with Qt 5.7 then to patch source-code on the fly. Developers should see and report the errors (warnings) upstream. I think the patching is overestimating the importance of these warnings. If the argument is that a standard user wouldn't be using the rewrites, then the severity argument is flawed. Users will build and use the software they develop, no? If I'm wrong, please explain.

jasp00 · 2017-01-27T22:00:18Z

# TODO: Remove once Travis ships with Qt 5.7

We can add comments about which versions are fixed. Travis is not the only affected environment.

Developers should see and report the errors (warnings) upstream.

They will see the errors, they may report the errors, and they will use -DFIX_SYSTEM_HEADERS=ON or fix the headers themselves.

If the argument is that a standard user wouldn't be using the rewrites, then the severity argument is flawed.

A standard user will not use the rewrite because she will not get the error (warning). The special user that builds with Clang, warning-clean, without sudo powers or the knowledge to fix a header, will use -DFIX_SYSTEM_HEADERS=ON.

tresf · 2017-02-05T18:19:16Z

include/debug.h

 #ifdef LMMS_DEBUG
 	#include <assert.h>
 #else
+	// TODO: assert is a standard macro, we should choose another name


we should choose another name

Isn't this quite common practice though?

Isn't this quite common practice though?

Using assert for operations with side effects? Yes, it is a common error. We should not change the meaning of standard assert.

tresf · 2017-02-05T18:29:41Z

@jasp00 I'd like to merge this, but I think @lukas-w's proposal is cleaner for the headers. #3208 (comment)

jasp00 · 2017-02-13T11:38:19Z

I think @lukas-w's proposal is cleaner for the headers.

Adding a command line argument like --system-header-prefix=/usr/ should silence them.

Please, let us not hide bugs. I do not understand your skepticism. If a user wants to rewrite her headers, she must say "I want to rewrite my headers, fix them for me". What is wrong with that? Do we not provide other workarounds?

If we should not offer a rewriting option, then let me fix the Travis environment only. If fixing on setup is not acceptable, then let me add an extra repository so that fixed packages can be used. Let us build on a fixed environment.

jasp00 · 2017-03-28T09:21:07Z

Since fixing gig.h is required, I believe there are no objections to the rewrite option. I will merge later.

lukas-w · 2017-03-28T10:07:43Z

I believe there are no objections to the rewrite option

There are objections, please don't merge.

As far as I can see, both @tresf and I have objected to rewriting any of the two headers, and have also stated why we think so and have suggested multiple alternatives. Yet you write "No one is giving a justification not to rewrite headers". For the record, these are my reasons: In the Qt case, it's overkill to introduce this to merely fix a warning. Generally speaking, I strongly oppose fixing upstream bugs (let alone warnings) in our code, especially if upstream already fixed them. I don't like the complexity introduced with this kind of fixes, and I don't feel it's even our responsibility.

As for QVector: There are several methods of silencing the warning that don't involve regex-fixing a header. We can also just build with relaxed settings.

As for gig.h: Upstream has fixed this two years ago in version 4.0. We could just require that version for the GigPlayer plugin. Let's look into that option first.

jasp00 · 2017-03-28T11:23:45Z

Yes, before I showed there is no choice, @tresf and you did state an objection, overkill (excessive complexity), which is not a justification unless there is a simpler way to achieve the same result, and there is not. You have suggested several alternatives and I have tried them all. Without this solution, you are just banning environments.

There are several methods of silencing the warning that don't involve regex-fixing a header

And all of them could hide potential bugs.

We can also just build with relaxed settings.

Which means that we do not fix warnings in continuous integration.

We could just require that version for the GigPlayer plugin.

You are forbidding any usable Debian system (and Ubuntu, of course) with Clang. Is requiring a full package any simpler than rewriting a header?

I am repeating myself. This is an optional flag. Do we not have other workarounds? No one is giving a sensible justification. Please explain how to accomplish the same result in a simpler way. If you do not understand the code, then I add more comments.

tresf · 2017-03-28T20:52:28Z

excessive complexity [...] is not a justification unless there is a simpler way to achieve the same result

I'll reiterate the discussion...

qvector.h

Qt ships with a header that barks with a warning on clang compiler. If we want to leverage strict Travis-CI compiler settings for the Clang environment (I think we all do) we need to find a way to get past this warning.
```
/usr/include/x86_64-linux-gnu/qt5/QtCore/qvector.h:767:21: warning: destination
      for this 'memmove' call is a pointer to class containing a dynamic class
      'QPixmap'; vtable pointer will be overwritten [-Wdynamic-class-memaccess]
```
A proposal by @lukas-w is to instruct the compiler that this is a system header that we have no control over, either through an ifdef flag on each qvector.h include or alternately through a compile setting. @jasp00 is worried -Wno-dynamic-class-memaccess for the entire project is unsafe. @jasp00 also advises against --system-header-prefix=/foo/bar. I personally feel --system-header-prefix=/foo/bar is an adequate compromise.

gig.h

Gig player simply won't work on Clang unless updated. Some repos already have the updated GigPlayer, but Travis-CI does not. This affects users on many popular distributions building LMMS using Clang using GigPlayer -- something LMMS has never really offered prior to this PR.
```
/usr/include/gig.h:434:27: error: ISO C++11 does not allow access declarations;
   use using declarations instead
```
A proposal by @lukas-w is to drop GigPlayer support < 4.0 as 4.0+ fixes this problem. @lukas-w did not specify if he meant drop it altogether or if he meant drop it only for Clang. @jasp00 has concerns that the entire feature shouldn't be dropped over a trivial header change. This concern has merit. I personally feel that GigPlayer is largely unused plugin which has never worked on these systems with Clang to begin with, so this is a few feature and CAN wait for upstream to fix it. For those rare occurrences that need GigPlayer without access to the newer version on the mentioned compiler, they can edit the header themselves, since the header is useless on Clang anyway. i.e. They're going to need it fixed for any other projects that use the header so why burden OUR project?

On a side note... whether we use it or not, @jasp00 I have to thank you for the work that has gone into this header patching as well as the patience and time taken to justify the changes however stating that all of the points above are not "sensible justification[s]" is weighing this feature higher than simplicity itself, which we have a clear divide on. This divide is going to be recurring and this divide is one which we'll need to overcome -- at least for this PR -- to get this into Travis. :)

jasp00 · 2017-03-29T23:48:10Z

Pending SWH fixes merged, swh/ladspa#47.

jasp00 · 2017-03-30T00:01:41Z

I am all for simplicity, but I am running out of alternatives. My concern is the Travis environment. May I just copy the headers in the Travis scripts? sha256sum plus cp, it is simple and we do more complex stuff.

Copy system headers and fix them Merge files from SWH upstream More Clang fixes

Fix sed expression

libgig's header uses non-C++11 standards. See also LMMS#3208

tresf · 2017-11-15T07:06:14Z

The decision was made not to rewrite system headers in this fashion, but mark the headers as SYSTEM headers and simplify the codebase.

There are several Clang fixes in here that haven't made it into the codebase, but many of them are for 3rd party LADSPA plugins and have since been moved to submodules. New PRs should be opened at the upstream homes, then we'll fast forward the submodules once accepted.

tresf reviewed Jan 4, 2017

View reviewed changes

tresf reviewed Jan 5, 2017

View reviewed changes

tresf mentioned this pull request Jan 7, 2017

Fix calf plugin compilation on Clang #3215

Merged

jasp00 force-pushed the clang branch from 23f13c7 to 5dbdd9f Compare January 7, 2017 18:46

jasp00 force-pushed the clang branch from 5dbdd9f to 510712d Compare January 24, 2017 14:46

tresf reviewed Feb 5, 2017

View reviewed changes

jasp00 force-pushed the clang branch 2 times, most recently from 00a348a to 143370c Compare March 28, 2017 02:27

tresf mentioned this pull request Apr 2, 2017

Add shellcheck to the build #3476

Merged

Javier Serrano Polo added 15 commits April 2, 2017 08:30

Fix build with Clang

1e9c589

Fix errors in builds with Clang at Travis CI

af9346e

Drop assert usage, behavior was different when enabled

99650cb

Add build with Clang and Qt 5

b1b8b02

Copy system headers and fix them Merge files from SWH upstream More Clang fixes

Drop build with Clang and broken Qt 4

c6124fe

Fix sed expression

Add parameter to fix system headers

2b930f2

Point to GitHub report

1677fe9

Fix ccache build

c3ee95b

Use Clang flags only with Clang and fix system headers

1ff71c0

Fix tautological compare

ef31381

Remove Clang flags for wineg++

2d08c45

Inform when rewriting headers

84cdc04

Debugging Travis...

5d81eb5

Fix rebase

31b1dd6

Stop debugging

4e89bc5

jasp00 force-pushed the clang branch from c9baf9f to 4e89bc5 Compare April 2, 2017 06:33

tresf added a commit to tresf/lmms that referenced this pull request Jun 1, 2017

Remove C++11 for GigPlayer

1dcfc99

libgig's header uses non-C++11 standards. See also LMMS#3208

tresf mentioned this pull request Jun 1, 2017

Add more brew packages #3595

Merged

tresf mentioned this pull request Oct 6, 2017

Add .clang-format file and format the src/ and include/ with it. #3843

Closed

tresf closed this Nov 15, 2017

Uh oh!

Fix build with Clang #3208

Fix build with Clang #3208

Uh oh!

Conversation

jasp00 commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tresf Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasp00 commented Jan 5, 2017

Uh oh!

tresf commented Jan 5, 2017

Uh oh!

jasp00 commented Jan 5, 2017

Uh oh!

jasp00 commented Jan 24, 2017

Uh oh!

tresf commented Jan 24, 2017

Uh oh!

jasp00 commented Jan 24, 2017

Uh oh!

tresf commented Jan 24, 2017

Uh oh!

jasp00 commented Jan 24, 2017

Uh oh!

tresf commented Jan 24, 2017

Uh oh!

jasp00 commented Jan 24, 2017

Uh oh!

lukas-w commented Jan 24, 2017

Uh oh!

tresf commented Jan 24, 2017

Uh oh!

jasp00 commented Jan 24, 2017

Uh oh!

tresf commented Jan 27, 2017

Uh oh!

jasp00 commented Jan 27, 2017

Uh oh!

tresf commented Jan 27, 2017

Uh oh!

lukas-w commented Jan 27, 2017

Uh oh!

tresf commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasp00 commented Jan 27, 2017

Uh oh!

tresf commented Jan 27, 2017

Uh oh!

jasp00 commented Jan 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tresf commented Feb 5, 2017

Uh oh!

jasp00 commented Feb 13, 2017

Uh oh!

tresf Jan 5, 2017 •

edited

Loading

tresf commented Jan 27, 2017 •

edited

Loading

tresf commented Mar 28, 2017 •

edited

Loading