qtcreator 4.9.1 -> 4.10.0 #70573

deadloko · 2019-10-07T05:20:23Z

Motivation for this change

There was discussion with @SRGOM about upgrade on
#61348

Things done

Removed patches that was accepted into upstream.
Fixed linkage error: all modules that is linking with clang tooling libraries should have -fno-rtti flag as clang tooling libraries is built with static linkage.
Updated patch to find clang libc++ includes.
Added withClangPlugins option to make it possible to compile qtc without vendor clang dependency.

Notify maintainers

cc @akaWolf

SRGOM · 2019-10-12T07:38:17Z

I thought I commented, not sure why it didn't get posted...

Recommenting now....

So this is obviously a big changeset, and as I see- this has two components:

The easy part- version and hash change.
The crazy difficult part, which is the clang/clazy part. Since you introduced it, my guess would be that you've already tested it to your satisfaction. So probably good to go.

That said, may I request that you mention some steps as to how to go about making these changes for the future versions. Or perhaps make the clazy version optional?

deadloko · 2019-10-12T09:53:22Z

Clang is critical in sense that without it qtcreator becoming just text editor, i.e. no code completion, no symbol searches etc. Qt team manages there own repos of llvm, clang+clang-tools-extra+clazy is just one repository with submodules, so i don't think we need to turn it optional.

Now to part how i was figuring out what to do:

Changed version and hash and just tried to build it, saw some patches no longer applying cause they was included in new release.
Removed patches that was included and tried to build again, saw several linking errors like:

undefined reference to typeinfo for clang::tooling::CompilationDatabase

After googling found out that if you mix no-rtti libraries with rtti libraries there is similar linker errors, after that read manuals about rtti to figure out why it is needed. In short qtcreator need it cause there is some dynamic_cast in code. Added flag to disable it only for modules that are linking with llvm.
Somewhere in 3 found out that there is new variable LIBTOOLING_LIBS was introduced, added it to substitutions to found correct clang libraries
Compilation was successful, so i opened up test project in qtcreator only to see that clang code model fails to find clang includes.
After some code inspection i found out that qtcreator searches for includes in directory that is bundled with it, to make it so system clang don't interfere with search pathes there is method in code removeClangSystemHeaderPaths(builtInHeaderPaths);. In our case we want qtcreator to search for clang system include, so i removed it.
Recompiled and tested that now qtcreator and code_model is working as expected: code completion working, clazy and clang-tidy analysis running and reporting results, project compiles with both gcc and clang compilers. Little side note: it was necessary to modify kits and choose -wraped versions of compilers.

SRGOM · 2019-10-13T08:19:26Z

It's improper for me to mention it since you've spent hours getting this to work. But QtCreator has great debugging capabilities and a decent LSP server :).

The reason why I mentioned making this feature optional on a param is to set the right expectations for the update. When, say, qt 4.11 comes around, I can make the hash update and cover a decent percent of crowd (using it for debugger/LSP et al) but wouldn't be able to invest all the time you did. When that happens, people would be disappointed that the upgrade lost them features.

deadloko · 2019-10-14T12:54:20Z

Ok, i can see your point now, I've updated nix expression with withClangPlugins option. With this option it's possible now to build qtcreator without clang dependency at all (it will just skip compilation of all plugins that needs clang). I've also checked that test project still opens and compiles like intended.

SRGOM · 2019-10-18T08:50:30Z

Thank you. Btw, I don't have approved permissions or would have approved a week back :)

SRGOM · 2019-10-19T05:42:30Z

@teto Could you please merge if you're okay with akaWolf's and my approval? I'm new and I don't know what's the standard practice when a maintainer doesn't have write access to the repository. Sorry for bothering you out of the blue!

nixos-discourse · 2019-10-25T11:51:13Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/62

timokau · 2019-10-28T12:04:45Z

pkgs/development/tools/qtcreator/0001-Fix-clang-libcpp-regexp.patch

+
+ void HeaderPathFilter::tweakHeaderPaths()
+ {
+-    removeClangSystemHeaderPaths(builtInHeaderPaths);


Can you explain this change? It doesn't look like a modification to a regex.

I mentioned this in my previous comment.

Compilation was successful, so i opened up test project in qtcreator only to see that clang code model fails to find clang includes.
After some code inspection i found out that qtcreator searches for includes in directory that is bundled with it, to make it so system clang don't interfere with search pathes there is method in code removeClangSystemHeaderPaths(builtInHeaderPaths);. In our case we want qtcreator to search for clang system include, so i removed it.

And why is this combined with the already existing Fix-clang-libcpp-regex patch?

Cause it's solves the same problem, i may split it into two different patches or rename patch if needed.

It should be very obvious which patches are applied for which reason. Otherwise future maintainers won't be able to adapt or remove them. So adding an explanation to the comment and/or giving it a more descriptive name would be nice.

Looking at the code [1], it appears that "removeClangSystemHeaderPaths" removes the system clang include directories. It's not clear at all why we should disable this. Perhaps it is supposed to be able to find includes from clang_qt_vendor instead?

[1] https://github.com/qt-creator/qt-creator/blob/master/src/plugins/cpptools/headerpathfilter.cpp

Also is this an issue that is seen in 4.10.0 only or 4.9.1 as well?

Ok, i've splitted up the patch into two different patches and added comment.

Looking at the code [1], it appears that "removeClangSystemHeaderPaths" removes the system clang include directories. It's not clear at all why we should disable this. Perhaps it is supposed to be able to find includes from clang_qt_vendor instead?

As i previously mentioned qtcreator in classic linux packaging comes with clang, clazy and it's includes in one single package. Path for it is something like /usr/share/qtcreator/include ...
In our case we reuse system clang recipe to build different sources for clang, so it's pathes still found by system paths.

Also is this an issue that is seen in 4.10.0 only or 4.9.1 as well?

They changed behavior, previously system paths was removed only if there is "darwin" in system triple, as i don't have mac to test it i leave'd it untouched. So 4.9.1 worked as intended.

veprbl · 2019-11-02T07:04:04Z

I think you mixed up filenames. There shouldn't be a change to the "0001-Fix-clang-libcpp-regexp.patch" file.

deadloko · 2019-11-02T08:00:25Z

I think you mixed up filenames. There shouldn't be a change to the "0001-Fix-clang-libcpp-regexp.patch" file.

You're right, but there is change, because of filechanges of patched files (line numbers), fixed it.

timokau

Fine by me if @veprbl agrees.

SRGOM · 2019-11-02T13:18:18Z

I know I'm sounding like a broken record but if the default value of withClangPlugins were false, we could add qtcreator to the (new to me) r-ryantm updater and get qtcreator to update itself more periodically. When I sent the PR 3 months back, it had been stale for a year...

veprbl · 2019-11-03T23:47:41Z

@GrahamcOfBorg eval

deadloko · 2019-11-05T02:05:08Z

I know I'm sounding like a broken record but if the default value of withClangPlugins were false, we could add qtcreator to the (new to me) r-ryantm updater and get qtcreator to update itself more periodically. When I sent the PR 3 months back, it had been stale for a year...

Question is: will hydra build package with all options, or only default ones? Because of clang it will take about 1 hour to fully build clang + qtcreator, so it's not very user friendly. Personally, i think it's better to build ide with full functionality, especially core one like clang code model.

veprbl · 2019-11-05T03:55:43Z

I know I'm sounding like a broken record but if the default value of withClangPlugins were false, we could add qtcreator to the (new to me) r-ryantm updater and get qtcreator to update itself more periodically. When I sent the PR 3 months back, it had been stale for a year...

I don't think withClangPlugins is the problem here.

Looking at the r-ryantm's logs at https://discourse.nixos.org/t/nixpkgs-update-r-ryantm-logs/1464/44 I see:

2019-11-02T23:50:08 qtcreator 4.9.1 -> 4.10.1
2019-11-02T23:50:14 FAIL Old version not present in master derivation file.

This was thrown from https://github.com/ryantm/nixpkgs-update/blob/master/src/Nix.hs#L242
It seems like the problem is that the bot is not able to see string "4.9.1" because the version is concatenated from two bits "4.9" and "1":

nixpkgs/pkgs/development/tools/qtcreator/default.nix

Line 26 in 02fb0b9

version = "${baseVersion}.${revision}";

What needs to be done (as a first step at least) is version = "4.9.1" should be defined first and the value of baseVersion has to be derived from it.

deadloko · 2019-11-05T05:19:39Z

I know I'm sounding like a broken record but if the default value of withClangPlugins were false, we could add qtcreator to the (new to me) r-ryantm updater and get qtcreator to update itself more periodically. When I sent the PR 3 months back, it had been stale for a year...

I don't think withClangPlugins is the problem here.

Looking at the r-ryantm's logs at https://discourse.nixos.org/t/nixpkgs-update-r-ryantm-logs/1464/44 I see:
2019-11-02T23:50:08 qtcreator 4.9.1 -> 4.10.1
2019-11-02T23:50:14 FAIL Old version not present in master derivation file.
This was thrown from https://github.com/ryantm/nixpkgs-update/blob/master/src/Nix.hs#L242
It seems like the problem is that the bot is not able to see string "4.9.1" because the version is concatenated from two bits "4.9" and "1":

nixpkgs/pkgs/development/tools/qtcreator/default.nix

Line 26 in 02fb0b9

version = "${baseVersion}.${revision}";

What needs to be done (as a first step at least) is version = "4.9.1" should be defined first and the value of baseVersion has to be derived from it.

What is the proper way to do it? I still have little expirience in nix DSL, something i have in mind:

nix-repl> version = "4.10.0"                                                                                                          

nix-repl> baseVersion = "${builtins.elemAt (builtins.splitVersion(version)) 0}.${builtins.elemAt (builtins.splitVersion(version)) 1}"

nix-repl> baseVersion
"4.10"

veprbl · 2019-11-05T05:27:47Z

Maybe

builtins.concatStringsSep "." (lib.take 2 (builtins.splitVersion "4.10.1"))

Added optional withClangPlugins to disable clang plugins compilation and, therefore, vendor clang dependency.

deadloko · 2019-11-05T11:44:27Z

Maybe

builtins.concatStringsSep "." (lib.take 2 (builtins.splitVersion "4.10.1"))

Thanks, i've updated mr to include your remarks.

veprbl

I can confirm that the 0002-Dont-remove-clang-header-paths.patch indeed fixes the code completion, but I don't know why. I also would like to point out that ArchLinux uses stock clang[1] and compiles clazy plugin separately[2] and they don't need this patch. I think, we should be looking to do the same and get rid of this withClangPlugins option, which, in my opinion, is not very useful.

[1] https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/qtcreator
[2] https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/clazy

veprbl · 2019-11-05T15:34:25Z

@GrahamcOfBorg build qtcreator

deadloko · 2019-11-05T16:35:14Z

I can confirm that the 0002-Dont-remove-clang-header-paths.patch indeed fixes the code completion, but I don't know why. I also would like to point out that Archlinux uses stock clang[1] and compiles clazy plugin separately[2] and they don't need this patch. I think we should be looking to do the same and get rid of this withClangPlugins option, which, in my opinion, is not very useful.

[1] https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/qtcreator
[2] https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/clazy

I don't have archlinux installed, so i can't confirm if they have code model working. To get rid of clang fork i see a need for multiple mr's:

Add nix expression for clazy, as there is none now.
Try to build qtcreator with stock clang, last time i've tried this there was problems even with clang-format plugin.

As of withClangPlugins initial though for this was - to let people that want new qtcreator version asap, but don't have time to inspect why clang plugins won't build and don't need code model, to build qtcreator without any clang dependencies.

tl;dr changes needed to get rid of clang fork will take time and multiple mr's, for now, in my opinion, it's ok to merge this changes.

veprbl · 2019-11-05T17:08:05Z

@deadloko Thanks!

Added optional withClangPlugins to disable clang plugins compilation and, therefore, vendor clang dependency. (cherry picked from commit 1eafac2)

deadloko · 2019-11-06T02:10:34Z

@veprbl nice, thanks!

ofborg bot requested a review from akaWolf October 7, 2019 05:32

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Oct 7, 2019

deadloko force-pushed the update/qtcreator-4.10.0 branch from 2d1c4ca to c000bd7 Compare October 14, 2019 12:49

SRGOM approved these changes Oct 18, 2019

View reviewed changes

akaWolf approved these changes Oct 18, 2019

View reviewed changes

timokau reviewed Oct 28, 2019

View reviewed changes

deadloko force-pushed the update/qtcreator-4.10.0 branch from c000bd7 to 77c9fc4 Compare November 2, 2019 05:13

ofborg bot requested a review from akaWolf November 2, 2019 05:27

deadloko force-pushed the update/qtcreator-4.10.0 branch from 77c9fc4 to dfe88cf Compare November 2, 2019 07:58

timokau approved these changes Nov 2, 2019

View reviewed changes

veprbl self-assigned this Nov 2, 2019

qtcreator 4.9.1 -> 4.10.0

8ca4cee

Added optional withClangPlugins to disable clang plugins compilation and, therefore, vendor clang dependency.

deadloko force-pushed the update/qtcreator-4.10.0 branch from dfe88cf to 8ca4cee Compare November 5, 2019 11:43

veprbl approved these changes Nov 5, 2019

View reviewed changes

veprbl merged commit 1eafac2 into NixOS:master Nov 5, 2019

veprbl removed their assignment Nov 5, 2019

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 5, 2019

qtcreator 4.9.1 -> 4.10.0 (NixOS#70573)

f6135b1

Added optional withClangPlugins to disable clang plugins compilation and, therefore, vendor clang dependency. (cherry picked from commit 1eafac2)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qtcreator 4.9.1 -> 4.10.0 #70573

qtcreator 4.9.1 -> 4.10.0 #70573

deadloko commented Oct 7, 2019 •

edited

SRGOM commented Oct 12, 2019 •

edited

deadloko commented Oct 12, 2019 •

edited

SRGOM commented Oct 13, 2019

deadloko commented Oct 14, 2019

SRGOM commented Oct 18, 2019

SRGOM commented Oct 19, 2019 •

edited

nixos-discourse commented Oct 25, 2019

timokau Oct 28, 2019

deadloko Oct 28, 2019

timokau Oct 28, 2019

deadloko Oct 29, 2019

timokau Oct 29, 2019

veprbl Nov 1, 2019

veprbl Nov 1, 2019

deadloko Nov 2, 2019 •

edited

veprbl commented Nov 2, 2019

deadloko commented Nov 2, 2019

timokau left a comment

SRGOM commented Nov 2, 2019

veprbl commented Nov 3, 2019

deadloko commented Nov 5, 2019 •

edited

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl left a comment •

edited

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl commented Nov 5, 2019

deadloko commented Nov 6, 2019

qtcreator 4.9.1 -> 4.10.0 #70573

qtcreator 4.9.1 -> 4.10.0 #70573

Conversation

deadloko commented Oct 7, 2019 • edited

Motivation for this change

Things done

Notify maintainers

SRGOM commented Oct 12, 2019 • edited

deadloko commented Oct 12, 2019 • edited

SRGOM commented Oct 13, 2019

deadloko commented Oct 14, 2019

SRGOM commented Oct 18, 2019

SRGOM commented Oct 19, 2019 • edited

nixos-discourse commented Oct 25, 2019

timokau Oct 28, 2019

Choose a reason for hiding this comment

deadloko Oct 28, 2019

Choose a reason for hiding this comment

timokau Oct 28, 2019

Choose a reason for hiding this comment

deadloko Oct 29, 2019

Choose a reason for hiding this comment

timokau Oct 29, 2019

Choose a reason for hiding this comment

veprbl Nov 1, 2019

Choose a reason for hiding this comment

veprbl Nov 1, 2019

Choose a reason for hiding this comment

deadloko Nov 2, 2019 • edited

Choose a reason for hiding this comment

veprbl commented Nov 2, 2019

deadloko commented Nov 2, 2019

timokau left a comment

Choose a reason for hiding this comment

SRGOM commented Nov 2, 2019

veprbl commented Nov 3, 2019

deadloko commented Nov 5, 2019 • edited

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl left a comment • edited

Choose a reason for hiding this comment

veprbl commented Nov 5, 2019

deadloko commented Nov 5, 2019

veprbl commented Nov 5, 2019

deadloko commented Nov 6, 2019

deadloko commented Oct 7, 2019 •

edited

SRGOM commented Oct 12, 2019 •

edited

deadloko commented Oct 12, 2019 •

edited

SRGOM commented Oct 19, 2019 •

edited

deadloko Nov 2, 2019 •

edited

deadloko commented Nov 5, 2019 •

edited

veprbl left a comment •

edited