Skip to content
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

Feature/qt clang integration #61348

Merged
merged 1 commit into from Jun 10, 2019

Conversation

@deadloko
Copy link
Contributor

commented May 12, 2019

Motivation for this change

Starting with version 4.7 clang code model is the default method for supporting QOL services as is:
code completion, syntactic and semantic highlighting, diagnostics with fixits and integrated Clang-Tidy and Clazy checks, follow symbol, outline of symbols, tooltips, renaming of local symbols.

For future releases of qtc clang code model support is esssential need.

Things done
  • Clang:
    • Built as build dependency from vendor branches, that's why:
      • Clazy included to build and builds as plugin
      • Clazy and Clang-tidy added to driver, so they may be invoked in runtime as plugins through clang arguments
    • QtCreator:
    • Patched regexp for includes path of clang lib++ (to include paths like libc++-version/include, was libc++/include)
    • Added llvm/libclang dependencies for clang code model
    • Added clang dependencies for runtime analysis.
    • Added substitutes to clang clang*.pr* for qmake to find llvm/clang libraries and includes
    • Added substitutes to correct clazy plugin name. That changed with clang 8 release. Bug in archlinux tracker. Fixed in upstream but didn't make to release.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

How clang-tools and clang-code-model functionallity was tested
  1. Create shell.nix:
    with import <nixpkgs> {};
    
    {  
        environment = stdenvNoCC.mkDerivation {
        name = "dev_environment";
        buildInputs = [ qt5.full qtcreator cmake gcc8 clang_8 gdb boost libpqxx xterm bashInteractive 
    llvmPackages.libcxx];
      };
    }
    clang_8 is optional as qtcreator will still find it.
  2. Clone test project
  3. Launch nix-shell: nix-shell -I nixpkgs=../nixpkgs --pure --keep LD_LIBRARY_PATH
    nixpkgs path is path to my nixpkgs fork with this changes.
  4. Launch qtcreator: qtcreator, if debug of clang-model is needed: QT_LOGGING_RULES=qtc.clang*=true LIBCLANG_TIMING=1 qtcreator
  5. In settings create 2 kits: 1 for clang and 1 for gcc,
  6. Test that project is successfully building with this kits. And that after opening c++ source file there is no popups with errors.
  7. Launch clang tools analysis (In top menu Analysis->Clang-Tidy and Clazy...->Analyze) and check that there is warnings from diagnostic and no errors in application output tab 11:40:47: Clang-Tidy and Clazy finished: Processed 3 files successfully, 0 failed..

Update:

Rebased and tested on 03 june 2019 - reason is qtcreator was updated to 4.9.1 in master.

Update2:

As @abbradar adviced, i removed all changes that was made to system clang, added qt vendor clang version as build dependency. After testing all looks good: clang code model working with both clang and gcc compilers set in project kit. Clang-tidy and clazy checks also works like they should.

@deadloko deadloko requested a review from matthewbauer as a code owner May 12, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

As i am still newbie to nix and nixos, i have some doubts:

  • Is it ok to depend on specific version of llvm libraries?
  • Is it ok to add depends for compiler or should i resolve qtc need for clang plugins in some other way?
  • What is preferred method to change package sources? Instead of substituteInPlace i may create yet another .patch file.
  • Is there a name convention for .patch files?
@@ -0,0 +1,334 @@
diff --git a/tools/driver/CMakeLists.txt b/tools/driver/CMakeLists.txt

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer May 12, 2019

Member

Is there an upstream version of this patch? If possible it would be preferred to use fetchpatch to avoid checking this in.

This comment has been minimized.

Copy link
@deadloko

deadloko May 12, 2019

Author Contributor

Unfortunately no, qt maintain separate version of clang in their own git, and afaik deliver it through their installer package (binary .run file). It's is possible, however, to fetch patch directly from their tracker by link. Please let me know if i should do it this way.

This comment has been minimized.

Copy link
@matthewbauer

matthewbauer May 12, 2019

Member

Yeah that works

This comment has been minimized.

Copy link
@deadloko

deadloko May 13, 2019

Author Contributor

I tried using fetchpatch, but that fails (that diff is against older version of clang). I suggest leaving patch file as it is now.

@matthewbauer

This comment has been minimized.

Copy link
Member

commented May 12, 2019

  • Is it ok to depend on specific version of llvm libraries?

For something like qtcreator it is probably okay. Usually there are tight version constraints on apps like this anyway.

  • Is it ok to add depends for compiler or should i resolve qtc need for clang plugins in some other way?

This should be fine. Sometimes we can add runtime search paths for this kind of thing but llvm should be okay.

  • What is preferred method to change package sources? Instead of substituteInPlace i may create yet another .patch file.

.patch should definitely be used if you have something that is upstreamable. substituteInPlace is probably ok for cases like this where you are putting output paths in the source code. Ideally, packages would have configure options to set things like this (for instance, location of llvm), but it's not always worth maintaining.

  • Is there a name convention for .patch files?

No, but maybe there should be. If there's an upstream version, try to keep that name though.

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

  • Is it ok to depend on specific version of llvm libraries?

For something like qtcreator it is probably okay. Usually there are tight version constraints on apps like this anyway.

  • Is it ok to add depends for compiler or should i resolve qtc need for clang plugins in some other way?

This should be fine. Sometimes we can add runtime search paths for this kind of thing but llvm should be okay.

  • What is preferred method to change package sources? Instead of substituteInPlace i may create yet another .patch file.

.patch should definitely be used if you have something that is upstreamable. substituteInPlace is probably ok for cases like this where you are putting output paths in the source code. Ideally, packages would have configure options to set things like this (for instance, location of llvm), but it's not always worth maintaining.

  • Is there a name convention for .patch files?

No, but maybe there should be. If there's an upstream version, try to keep that name though.

Thank you for your answers.

@@ -16,14 +16,15 @@ stdenv.mkDerivation rec {

src = fetchurl {
url = "http://download.qt-project.org/official_releases/qtcreator/${baseVersion}/${version}/qt-creator-opensource-src-${version}.tar.xz";
sha256 = "1k23i1qsw6d06sy7g0vd699rbvwv6vbw211fy0nn0705a5zndbxv";
sha256 = "1a76jv1nl8fqgifk9pq5sdyrsq0ba9iwz5myz1898xhvaf91kvj6";

This comment has been minimized.

Copy link
@danieldk

danieldk May 14, 2019

Member

The updated hash should go into the commit that bumps the qtcreator version.

This comment has been minimized.

Copy link
@deadloko

deadloko May 15, 2019

Author Contributor

You're right. Fixed.

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from d097c00 to fb741fe May 15, 2019

@deadloko deadloko referenced this pull request May 15, 2019
0 of 10 tasks complete

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from fb741fe to f7603a9 May 15, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Removed patch for botan, it's deprecated.

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from f7603a9 to a241ce6 May 15, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

@nixos-discourse

This comment has been minimized.

Copy link

commented May 30, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/5

@matthewbauer matthewbauer requested review from Ericson2314 and abbradar Jun 3, 2019

@abbradar
Copy link
Member

left a comment

Awesome work! I have several questions regarding changes, see review.

pkgs/development/tools/qtcreator/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/qtcreator/default.nix Outdated Show resolved Hide resolved
diff --git a/tools/extra/clazy/CMakeLists.txt b/tools/extra/clazy/CMakeLists.txt
index d61714d0..f563976f 100644
--- a/tools/extra/clazy/CMakeLists.txt
+++ b/tools/extra/clazy/CMakeLists.txt

This comment has been minimized.

Copy link
@abbradar

abbradar Jun 3, 2019

Member

You almost completely replace clazys original CMake file with this. Where does it come from and why is it needed?

This comment has been minimized.

Copy link
@deadloko

deadloko Jun 3, 2019

Author Contributor

We need to build clazy as clang plugin, not stand alone version, to be later invoked from clang executable.

This comment has been minimized.

Copy link
@abbradar

abbradar Jun 3, 2019

Member

Okay, but where does the modified file come from? Qt Creator's fork? It might be a good idea to explain it as a comment in expression along with a link to the original patch (which doesn't change this CMakeLists.txt).

This comment has been minimized.

Copy link
@deadloko

deadloko Jun 3, 2019

Author Contributor

I wrote it myself (actually just deleted all calls that is not needed for plugin build)

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from a241ce6 to b8596bb Jun 3, 2019

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

After thinking some more about this: @deadloko maybe we should just build Qt's Clang fork separately? I feel that all this patching is quite flaky and more problems may possibly come later (maybe Qt Creator would expect additional information from clazy or something else). OTOH the fork is guaranteed to be good and doesn't require all this patching. One downside is additional compile time and a separate Clang (but I feel closure sizes aren't a problem for Qt Creator).

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@abbradar This is one of possible solutions for this. I'll invest some more time to see if it's possible, cause i've never tried to build qtc with qt version of llvm. I'm still have little experience with nix expressions, but, hopefully, i won't need to to write them from scratch. I'd like to have some advice on:

  • Where should .nix files for this fork reside (pkgs/development/compilers/ is the location that i have in mind)? And how should i name it?
  • Is it ok to override llvm,clang, etc... for this fork (I think it should be able to build just fine with just src attribute override) or should i use some different approach?
@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

If just an src override to clang would be enough (try it!) I'd put it into a let in Qt Creator's expression. BTW do they have a customized version of LLVM too? I'd try to build just Clang fork first, using system LLVM.

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

They have whole llvm project (llvm, clang, clang-tools-extra) forked, but i think system LLVM should do just fine.

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Little update:
I managed to build clang as qtcreator runtime dependency for clazy and clang-tidy analysis, but i need some time to test it.
Unfortunately clang-lazy -> clazy patch still needed. They fixed it in qtcreator development tree, but it didn't make it into 4.9.1 release. So i would just add comment about it.

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from b8596bb to 668831c Jun 6, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@abbradar I would appreciate if you review new changes.

@abbradar
Copy link
Member

left a comment

Small fixes and we're good to go. Now we can merge it without fearing that clang breaks somehow, thanks!

pkgs/development/tools/qtcreator/default.nix Outdated Show resolved Hide resolved

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from 668831c to 4a0a8f4 Jun 6, 2019

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@deadloko I tried to build it but got invalid checksum errors for patches, which is a bad sign:

hash mismatch in fixed-output derivation '/nix/store/6az7n3nyaz6nks71a7z0jhv9mld8lj10-clangtidyclazyrunner.cpp?id=53c407bc0c87e0b65b537bf26836ddd8e00ead82':
  wanted: sha256:1nqpwc6b421rvwv64wbpfaykv7fxrn9r1qrw1xf6cl5hqhnw7y93
  got:    sha256:1rl0rc2l297lpfhhawvkkmj77zb081hhp0bbi7nnykf3q9ch0clh

Can you try redownloading them? I think there was a better way than collecting garbage and building it again recently but not sure.

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from 4a0a8f4 to f3a7f76 Jun 7, 2019

qtcreator: add clang code model compilation
Clang code model provides user with services: code completion,
syntactic and semantic highlighting, diagnostics, outline of symbols,
tooltips, renaming of local symbols. It's naturally depends on llvm and clang.
To make it work with nixos:
1. Add qt version of clang as build dependency.
2. Added patch for clang libc++ regexp trunk regexp not including path
like "libc++-version".
3. Fixed paths to llvm/clang libraries and includes.
4. Fixed name of clazy clang plugin.

@deadloko deadloko force-pushed the deadloko:feature/QtClangIntegration branch from f3a7f76 to 3d80880 Jun 7, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@abbradar I fixed hashes for patches, but it's strange behavior.

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@abbradar I've tested build on another machine (NixOS too). Build still succeedes.

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@deadloko

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@abbradar Well, that's unfortunate, hope your cpu replacement soon. =) Also can't you ask borg bot to build this one?
@danieldk @matthewbauer @Ericson2314 may i ask you to provide aid on this one?

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I'd just like to run it too but as I can't and you tested it - let's just check that it builds

@GrahamcOfBorg build qtcreator

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Bot's timeout kicked in ;_; Can someone try to build this?

@abbradar

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Let's not keep this stalled - you tested it on two machines so this should be enough. Merging, thanks!

@abbradar abbradar merged commit 6365fa1 into NixOS:master Jun 10, 2019

14 of 16 checks passed

qtcreator on x86_64-darwin No attempt
Details
qtcreator on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
qtcreator on aarch64-linux Success
Details
@SRGOM

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

For me this version 4.9.1 (on one machine) fails to start with (works fine on another): qt.qpa.plugin: Could not find the Qt platform plugin "xcb" in "" This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

I want to help but I'm not sure what information to provide here, seeing as working on one but not on another is pretty much against Nix fundamentally (irrespective of versions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.