Also
- require cmake 3.0 as elsewhere in kdev world
- mark ItemModels for future removal
- check for cargo as runtime dep
- use module specific KDEV_CARGO id for logged debugging
Details
Details
Diff Detail
Diff Detail
- Repository
- R894 KDevelop Cargo Support
- Branch
- pimpcmake
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Would commit as separate commits, but phabricator sadly does not yet support review of commit sets.
Comment Actions
Looks good, thanks for this change. I agree that this plugin should be more in line with the rest of KDevelop.
But regarding multiple commints, from as little as I saw about Phabricator, you can use arc to generate a diff from multiple commits, and paste that diff here. Although I never tried it, doesn't that produce the same result as a single bigger commit? The FindCargo thing is mostly unrelated to the other changes.
The other thing is working with raw diffs is annoying if I want to test the change. In the future, can you just push the branch so I can check it out locally?
Comment Actions
And there's no button here to merge this change, right? You have to do that manually?
Comment Actions
Yes. Phabricator doesn't provide this feature. You have to land this patch manually (via arc or plain git push).
Comment Actions
Two problem here IMHO (as remembered from the last time I played with options):
- arc (ab)uses the commit message of the last commit in the range diff'ed to store the review message for the whole range
- in the review request the reviewer still has no chance to see and comment on the individual commits
I think Phabricator here is really lacking, but moaning does not change facts or help anyone, so I back out to my hope that someone else will have the time and energy to improve things(tm) for everyone, by whatever.
The other thing is working with raw diffs is annoying if I want to test the change. In the future, can you just push the branch so I can check it out locally?
The workflow the phabricator developers want us to do here is to use arc patch Dxyz to get the diff from the server and to have it locally applied. If possible please try for now to get used to that.
Because while I agree that having the real developer branches available (like e.g. known with github) is a very useful thing to have, in the KDE infrasstructure setup developers usually do not have their personal repo clone, and the main repo does not allow forced pushes or deletion of remote branches (only when really needed). Now at least my personal workflow (but I have seen it with many) is to keep working copy branches including those already under review always rebased against latest master/target branch instead of merging that branch into those, as this way the branches tree is kept simple and clear. And given that the average review request has >1 iterations, any update of the branch with the commits under review (either for rebase or for feedback integration) is not possible right now, besides appending merges and fixup commits. So meh. Thus I would be happy to stay with my workflow if arc patch is sufficient for you for now.
Comment Actions
Committed as 23fdf1ece59725bf28e854c060f8dd2512364d72 ..74d62ab60ffcfbcfc1c4f76c2df30fad9c61d43f
(left out the find_package(KDevPlatform) change though, as cmake will fail instantly if dep not found because elsewhere macros are used which are available only if kdevplatform cmake config is found, so the intention of that change, to have cmake run to end and show missing stuff only in the summary log, is not achieved)
Comment Actions
PS: it might still make sense to bump the min dep of kdevplatform to 5.1 or even 5.2, but that is also a matter of planned releases and respective branches, so discussion post-poned to after your planned introductional email :)
Comment Actions
Thanks, I didn't know about the arc patch command, this does make it easy enough. Yeah, let's stick to the usual workflow.