Note: This is alternative approach to https://phabricator.kde.org/D15976

Hey @Petross404 -- the other day I reworked that script a bit, here's the result: https://phabricator.kde.org/D16123 -- I refactored it this way before seeing the new revision of yours. I do think my version is a little bit easier to grasp, since it does not have that many branches compared to yours.

I'm torn about this review; I'm with Aleix in the sense that this adds lots and lots of extra CMake code to maintain without a /lot/ of gain.

On some quick experiments I could not find a nice way to merge the generation of the .categories file into the declare macros. So when it comes to invested resources, I would rather prefer to keep the current explicit install_* macro calls for now, until someone has a better idea.

In D16032#340668, @apol wrote:

Considering we're doing our own macro,

In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

In D15899#336024, @apol wrote:

Would it make sense to recreate the KDirWatchers at this point? I would prefer if this patch didn't have to get outside the plugin.

Our script now initializes the environment correctly (at least for VS2017) even if the user doesn't run it from Developers Command Prompt. It reads Registry to find the path under which VS2017 is installed (I fear that I read the wrong variable from the Registry).

Otherwise looks good! If you feel super ambitious you could add a test, should be very simple ;)

Can a windows user with VS installed, give me the content of HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\VisualStudio\SxS\VS7?

update to latest master

I'd be pushing less if this hadn't already been committed and then reverted because of an issue that got through the initial review process.

In D9344#339729, @rjvbb wrote:

In absence of instructions to the contrary I'll interpret Milian's "feel free to respin" as "feel free to recommit", and push a commit during the day. This has been waiting to be re-applied long enough.

Can be committed this way as well, but I'd research using QScopedPoiner in some places instead.

Changed to (correct) ktexteditor repository. Someday... I will learn how to use phabricator :)

In absence of instructions to the contrary I'll interpret Milian's "feel free to respin" as "feel free to recommit", and push a commit during the day. This has been waiting to be re-applied long enough.

People with Visual Studio other than Community Edition, please consider trying out these changes. If a typo or a bug exists, it should be found early.

1. The script (sadly) uses hardcoded paths for vcvarsall.bat, instead of needing VS150COMNTOOLS. This way it can be found and executed, without the need of Developer's Command Prompt.

Ah, right, I missed that part. The problem is that VS2017 by default doesn't set VS150COMNTOOLS as system-wide env variable.

@kfunk As I already have mentioned, clicking the script (finding it in Start menu) doesn't initialize correctly the environment. It has to be run from within Developer Command Prompt or the complete path of MSCV binaries (for the wanted target) has to be in %PATH%.

Looks good to me, please push to 5.3 branch.

In D16012#338924, @asturmlechner wrote:

(there are also some missing deps that are implicitly linked, but that is for another review)

Pushed this for you. To 5.3 branch.

Initial commit message also had:
"The EXPORT argument is kept explicit for now, though it could be considered to
hardcode the related install_qt_logging_categories() calls into the two
macros."

(there are also some missing deps that are implicitly linked, but that is for another review)

Make KF5Archive exclusive to BUILD_TESTING as well

This did not work for templates as "isComment" uses line attributes and at the time the template is inserted it is always "C/C++ Code/Data"-attribute never a comment. It only becomes a "doxygen comment"-attribute after the template "editing" is finished.

In D16012#338544, @kfunk wrote:

To 5.3 branch, please.

To 5.3 branch, please.

Thanks for review,

Looks good to me in general, feel free to push to master.

I don't see why not.

Not sure about x64 part. What if user wants x32 project? Or that is arch of the toolchain itself?

Confirmed to work on openSUSE TW:

kdevelop.plugins.clang: Full Clang version: "clang version 6.0.1 (tags/RELEASE_601/final 335528)"
kdevelop.plugins.clang: Detected Clang version: "6.0.1"
kdevelop.plugins.clang: Using builtin dir: /usr/lib64/clang/6.0.1/include
kdevelop.plugins.clang: Using Clang builtin include path: "/usr/lib64/clang/6.0.1/include"

or when being nasty setting KDEV_CLANG_BUILTIN_DIR=/does/not/exist:

kdevelop.plugins.clang: Full Clang version: "clang version 6.0.1 (tags/RELEASE_601/final 335528)"
kdevelop.plugins.clang: Detected Clang version: "6.0.1"
kdevelop.plugins.clang: Using dir from $KDEV_CLANG_BUILTIN_DIR: "/does/not/exist"
kdevelop.plugins.clang: Using Clang builtin include path: "/does/not/exist"

which results in unloading the clang plugin, as expected.

Looks good to me, and with little risk ... on Linux nothing changes, on Windows we test it anyways before shipping.

You're right. For the Windows case, we wouldn't actually need to query the Clang version at runtime, since we control the version of Clang/LLVM ourselves.

Don't we know the exact version of Clang at compile-time when building the bundled version? I think this change is mainly interesting for the "unbundled" case when Clang can be updated independently. Or what am I missing?

Otoh, Clang 7 "fixes" this since they no longer use "X.Y.Z/include" but just "X/include" -- so we do not have to worry about this any longer anyway.

Note: Doing the retrieval at runtime would also allow distro packages to upgrade the libclang version under-the-hood; without having the need to recompile KDevelop. I.e. going from Clang/LLVM 6.0.0 to 6.0.1 (and keeping ABI), KDevelop would keep working just fine (which it currently wouldn't...).

Pushed via: https://commits.kde.org/kdevelop/716372ae2e8dff9c51e94d33443536786e4bd85b

Rest LGTM, feel free to push after fixing my remark.

Used PATH_SUFFIXES to cleanup the find_path a bit more

FreeBSD+Clang 6 also works fine with this patch.

But I think in general that approach makes more sense. Thanks for working on it.

In D15895#335917, @buschinski wrote:

"cpuid.h" on my system is in:

• /usr/lib64/clang/7.0.0/include/cpuid.h

$ llvm-config --libdir
/usr/lib64/llvm/7/lib64

I can tell the updated patch results does not change behaviour on openSUSE TW,,same value for KDEV_CLANG_BUILTIN_DIR before and after (/usr/lib64/clang/6.0.1/include).

On Arch Linux, the latest version of the patch seems to work fine.

Would it make sense to recreate the KDirWatchers at this point? I would prefer if this patch didn't have to get outside the plugin.

Added additional search PATHS with CLANG_LIBRARY_DIRS, like it was used in the original version (without this patch). This should hopefully work on opensuse as well.

"cpuid.h" on my system is in (listing ALL locations):

• /usr/lib64/clang/7.0.0/include/cpuid.h
• /usr/lib64/gcc/x86_64-pc-linux-gnu/7.3.0/include/cpuid.h
• /usr/lib64/gcc/x86_64-pc-linux-gnu/8.2.0/include/cpuid.h
• and a few in /usr/src/linux-* (linux kernel sources)

Forgot to mention: the parameter "--quiet" seems to be around since at least 2005, see date stamp on the docs of docutils 0.4: http://docutils.sourceforge.net/0.4/docs/user/config.html
So should be safe to use unconditionally.

Looks good, thanks!

Added a test.
But, as I've mentioned before, the tests for code completion crash,
So I haven't been able to run all tests, though you can run a single test case
before it crashes.

A bit sad i blew up the code here. But then when I first looked at the code I was wondering about all the corner cases, so being a bit more explicit might not hurt. Moving into separate method at least helps keeping the actual method small and understandable.
Second while loop done to be consistent a bit with first one, undecided wheter another loop would be better, so looking for opinions here :)

This looks sound but it would be very good if it included a unit test.