⚙ D734 CMake: Fix target enumerating on Windows. For some reason, $ at the end of regexp causes matches to be empty. While there, filter out internal CMake targets.

arrowd updated this revision to Diff 1712.Jan 3 2016, 5:34 PM

arrowd retitled this revision from to CMake: Fix target enumerating on Windows. For some reason, $ at the end of regexp causes matches to be empty. While there, filter out internal CMake targets..

arrowd updated this object.

arrowd edited the test plan for this revision. (Show Details)

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 3 2016, 5:34 PM

arrowd added reviewers: apol, kfunk.Jan 3 2016, 5:35 PM

arrowd added a project: KDevelop.

apol added inline comments.Jan 3 2016, 5:49 PM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	I don't understand why.
107	This should be in CMakeManager::populateTargets(), with the *_automoc target filtering.

arrowd added inline comments.Jan 3 2016, 5:57 PM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	Me neither. What you suggest?
107	K, will do.

apol added inline comments.Jan 3 2016, 6:05 PM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	You can explain a bit better why it doesn't match with the $. If you print the matched strings, what do you get?

Perform target filtering in CMakeManager::populateTargets()

arrowd marked an inline comment as done.Jan 3 2016, 6:14 PM

arrowd added inline comments.

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	They are empty, both of them.

Commit the filtering part, if you want. This is unrelated anyway.

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	What's the original string? Does it look like it should match?

arrowd added inline comments.Jan 3 2016, 7:15 PM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	Absolutely. C:/bla/bla/path/to/build/CMakeFiles/targetname.dir But no matches.

Note: Please unit test this.

Factor out the import logic, then in a unit test parse a small TargetDirectories.txt file and check whether the parse succeeded. Trivial.

apol added inline comments.Jan 4 2016, 1:09 AM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	Well, changing the line would mean to remove it because either we don't understand how QRegularExpression works or because ::readLine() isn't doint its job well (and thus should be properly fixed in Qt).

Optionally consume \r symbol at the end of line.

arrowd added inline comments.Jan 4 2016, 8:52 AM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	How about this? Even if it is a bug in Qt, this code wouldn't break once it is fixed.

kfunk added inline comments.Jan 4 2016, 9:48 AM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	Could also switch to `QTextStream`-based file reading, which strips \r\n and \n. Makes this code more readable and avoids us having to do implement platform-specific behavior. See: http://doc.qt.io/qt-5/qtextstream.html#readLine

Use QTextStream to trim lines from \r\n.

Perfect! +1

This revision is now accepted and ready to land.Jan 4 2016, 11:45 AM

kfunk added inline comments.Jan 4 2016, 11:46 AM

projectmanagers/cmake/cmakeimportjsonjob.cpp
105–106	Just `while(...)`

Looks good to me.

Add a testcase.

Tweak test logic.

kfunk added inline comments.Jan 4 2016, 6:11 PM

projectmanagers/cmake/tests/test_cmakemanager.cpp
304	Be explicit: Call it `testEnumerateTargets()`
308	`QTemporaryFile`?
320	Better use `QTemporaryDir("/path/to/subdir")` on the stack => takes care of construction & deletion of this dir

Rename test. Use QTemporary{File,Dir}.

Feel free to push after fixing those minor issues. Thanks!

projectmanagers/cmake/tests/test_cmakemanager.cpp
308	Cleaner to use absolute file paths here: `QDir::tempPath() + "/TargetDirectories.txt"` -> Doesn't pollute your CWD
309	Dito, using the default ctor of `QTemporaryDir` is fine already.

Commited in
http://commits.kde.org/kdevelop/1d17af052191d666c259dbda61314ade46d6a3d4
http://commits.kde.org/kdevelop/4de75c2a4633c8c3ef299c760cf4e2d7d75d5a5e

			Path	Packages
M			projectmanagers/cmake/cmakeimportjsonjob.cpp (22 lines)
M			projectmanagers/cmake/cmakemanager.cpp (2 lines)
M			projectmanagers/cmake/cmakeutils.h (5 lines)
M			projectmanagers/cmake/cmakeutils.cpp (23 lines)
M			projectmanagers/cmake/tests/test_cmakemanager.h (1 line)
M			projectmanagers/cmake/tests/test_cmakemanager.cpp (28 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	1712	cc6350b		Jan 3 2016, 5:34 PM	★	★
Diff 2	1715	cc6350b	Perform target filtering in CMakeManager::populateTargets()	Jan 3 2016, 6:14 PM	★	★
Diff 3	1718	cc6350b	Optionally consume \r symbol at the end of line.	Jan 4 2016, 8:50 AM	★	★
Diff 4	1723	cc6350b	Use QTextStream to trim lines from \r\n.	Jan 4 2016, 11:24 AM	★	★
Diff 5	1728	cc6350b	Add a testcase.	Jan 4 2016, 5:51 PM	★	★
Diff 6	1729	cc6350b	Tweak test logic.	Jan 4 2016, 5:53 PM	★	★
Diff 7	1731	cc6350b	Rename test. Use QTemporary{File,Dir}.	Jan 4 2016, 6:42 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
3b483a5bf3c2	f013cb85956c	1d17af052191	Gleb Popov	CMake: Fix target enumerating on Windows by using QTextStream for reading… (Show More…)	Jan 3 2016, 7:18 PM
1d17af052191	e2f68fd79d54	cc6350ba5174	Gleb Popov	Filter out internal CMake targets that appear on Windows.	Jan 3 2016, 5:13 PM

CMake: Fix target enumerating on Windows. For some reason, $ at the end of regexp causes matches to be empty. While there, filter out internal CMake targets.
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 1731

projectmanagers/cmake/cmakeimportjsonjob.cpp

projectmanagers/cmake/cmakemanager.cpp

projectmanagers/cmake/cmakeutils.h

projectmanagers/cmake/cmakeutils.cpp

projectmanagers/cmake/tests/test_cmakemanager.h

projectmanagers/cmake/tests/test_cmakemanager.cpp

CMake: Fix target enumerating on Windows. For some reason, $ at the end of regexp causes matches to be empty. While there, filter out internal CMake targets.ClosedPublicActions