Allow users to install via a custom root installation command
ClosedPublic

Authored by geetamc on Mar 21 2017, 5:32 PM.

Details

Summary

Fixes BUG: 374144

-In makeconfig.ui, made the combo box for root installation command editable
-In makebuilderconfig.kcfg changed suCommand's type to string from int.
-In makejob.cpp Added a simple regexp to split the input command appropriately so that arguments pass correctly.

Reasons to accept this patch:
-Users will be able to pass any argument that they want (or remove any they don't)
-Some distros ship those binaries with wierd names such as kdesu5

Reasons not to accept this patch:
-You can get random commands to execute by entering it in the editable combo box

Although you can get random commands to execute by entering them in konsole, but that isn't considered
a security flaw :)

Test Plan

I have gotten kdevelop itself to install just fine by custom command "kdesudo".

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
geetamc created this revision.Mar 21 2017, 5:32 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 21 2017, 5:32 PM
geetamc edited the summary of this revision. (Show Details)Mar 21 2017, 5:35 PM
brauch added a subscriber: brauch.Mar 21 2017, 5:36 PM

Can you make the line edit disabled when something else than "custom command" is selected?
Otherwise, LGTM, thanks for the patch!

projectbuilders/makebuilder/makeconfig.ui
285

should have a : at the end

projectbuilders/makebuilder/makejob.cpp
195

you can use {"kdesu", "-t"} these days, and {customCommand} in the previous line, but not an issue

kfunk requested changes to this revision.Mar 21 2017, 5:50 PM
kfunk added a subscriber: kfunk.
kfunk added inline comments.
projectbuilders/makebuilder/makeconfig.ui
124

Casing: Custom Command

projectbuilders/makebuilder/makejob.cpp
195

The else branch is not needed, let it fall-through.

This revision now requires changes to proceed.Mar 21 2017, 5:50 PM
In D5122#96534, @brauch wrote:

Can you make the line edit disabled when something else than "custom command" is selected?
Otherwise, LGTM, thanks for the patch!

Yeah sure, thanks a lot for reviewing.

geetamc added a comment.EditedMar 21 2017, 6:22 PM

My build fails right now after a git pull

kdevelop/languages/clang/duchain/macronavigationcontext.cpp:58:28: error: ‘prefix’ was not declared in this scope

I have made most of the changes also I have an exam tomorrow so I will definitely update the diff by tomorrow.

Thank you both brauch and kfunk for reviewing.

geetamc updated this revision to Diff 12668.Mar 21 2017, 6:46 PM
geetamc edited edge metadata.

-Corrected the casing of "Custom Command"
-Appended ':' to the label
-Label and text edit of Custom Command are now disabled by default
-removed unnecessary 'else'

kfunk added a comment.Mar 21 2017, 7:03 PM

My build fails right now after a git pull

kdevelop/languages/clang/duchain/macronavigationcontext.cpp:58:28: error: ‘prefix’ was not declared in this scope

You need to git pull in kdevplatform and recompile/reinstall.

I have made most of the changes also I have an exam tomorrow so I will definitely update the diff by tomorrow.

Thank you both brauch and kfunk for reviewing.

geetamc marked 3 inline comments as done.Mar 21 2017, 7:06 PM
In D5122#96551, @kfunk wrote:

My build fails right now after a git pull

kdevelop/languages/clang/duchain/macronavigationcontext.cpp:58:28: error: ‘prefix’ was not declared in this scope

You need to git pull in kdevplatform and recompile/reinstall.

I have made most of the changes also I have an exam tomorrow so I will definitely update the diff by tomorrow.

Thank you both brauch and kfunk for reviewing.

Sorry! Me forgetting to pull kdevplatform is rather more frequent than it should be.

The build was successful, the patch now works as intended please let me know if I missed something.

Since I am new to submitting patches would you please tell me when updating diff should I upload the diff representing the latest changes or one that represents the whole patch?

I forgot to implement a feature mentioned by brauch, will do it tomorrow.

Since I am new to submitting patches would you please tell me when updating diff should I upload the diff representing the latest changes or one that represents the whole patch?

The whole patch.

geetamc updated this revision to Diff 12682.EditedMar 22 2017, 11:45 AM
geetamc edited the summary of this revision. (Show Details)

I believe that having combo box editable is a cleaner way to solve the same problem, I will revert the changes if reviewers feel that having a plain text edit and an entry for Custom Command in combo box is more appropriate.

geetamc edited the summary of this revision. (Show Details)Mar 22 2017, 11:54 AM
geetamc updated this revision to Diff 12683.EditedMar 22 2017, 12:18 PM
geetamc edited the summary of this revision. (Show Details)

arguments such as "-t" now pass correctly.

geetamc edited the summary of this revision. (Show Details)Mar 22 2017, 12:20 PM
ematirov added inline comments.
projectbuilders/makebuilder/makejob.cpp
180

I wonder if that change will drop previous user selection, won't it? If so, maybe it's better to workaround that somehow.
Thus, in that scenario correct command should be chosen:

  1. I use previous version of KDevelop.
  2. I set su command to "kdesu".
  3. I update KDevelop to version after your patch.
  4. I expect that su command is still "kdesu".

@ematirov Thank you for reviewing! Have a look at the inline comment.

projectbuilders/makebuilder/makejob.cpp
180

Yes, you are right, what I have done is:

  1. Convert suCommand toInt(&ok, 10) and store it in a variable say, suCommandNum
  1. If ( ok ) then have a switch block on suCommandNum and accordingly set a variable suCommandStr.
  1. Write suCommandStr to Entry "Su Command". (so that next time ok becomes false)
  1. Read the Entry "Su Command" and store it in variable suCommand.

Rest remains the same suCommand when split returns a QStringList(), which is returned by the function if it isn't empty.

If everyone concerned is fine with it, I will update the diff.

geetamc added a comment.EditedMar 30 2017, 6:56 PM

Alright so I feel, regarding my last comment, Having an extra variable suCommandStr and an extra readEntry() is unnecessary, In the switch block I will simply assign suCommand the appropriate value, and write suCommand to the "Su Command" Entry.
Also I feel updating the diff will give you guys a better Idea so I will do the same in a while.

geetamc updated this revision to Diff 13012.Mar 30 2017, 7:10 PM
geetamc edited the summary of this revision. (Show Details)

If entry "Su Command" is an integer in range [0, 3) (which can be a scenario when user, for the first time opens his/her project in the updated kdevelop)
then suCommand is set appropriately and is written to entry "Su Command" .

Few suggestions related to the commit message: it should follow the guidelines for git message.
So the first line should be a short summary of the change (no need to mention the bug), "allow users to install via custom command" or something like that.

The bug should report in one of the other line not as link but with

BUG: nnnnnn

geetamc retitled this revision from This patch fixes bug 374144, Basically it allows users to install via a custom root installation command to Allow users to install via a custom root installation command.Mar 31 2017, 3:52 PM
geetamc edited the summary of this revision. (Show Details)
geetamc marked an inline comment as done.

Few suggestions related to the commit message: it should follow the guidelines for git message.
So the first line should be a short summary of the change (no need to mention the bug), "allow users to install via custom command" or something like that.

The bug should report in one of the other line not as link but with

BUG: nnnnnn

Thank you, I will keep that in mind from now on.

This revision now requires changes to proceed.Apr 27 2017, 3:20 PM
zhigalin accepted this revision.May 7 2017, 12:36 PM
zhigalin added a subscriber: zhigalin.

Hello and thanks for your effort.
Just a few considerations:

  • When you edit an UI file, make sure not to forget to increase it's version
  • You also forgot to bind kcfg_suCommand.enabled(bool) to kcfg_installAsRoot.toggled(bool)
  • You must use QStringLiteral, not just plain strings
  • Try placing less newlines around
projectbuilders/makebuilder/makejob.cpp
186

Unused!

This revision was automatically updated to reflect the committed changes.

Hello and thanks for your effort.
Just a few considerations:

  • When you edit an UI file, make sure not to forget to increase it's version
  • You also forgot to bind kcfg_suCommand.enabled(bool) to kcfg_installAsRoot.toggled(bool)
  • You must use QStringLiteral, not just plain strings
  • Try placing less newlines around

Thanks a lot for your suggestions!

  • When you edit an UI file, make sure not to forget to increase it's version

This is a valid comment for kxmlgui rc files, but not for Qt ui files :) The version attribute of the <ui> rather specifies the version of the schema used, not of the content.
Not sure what the effects of <ui version="4.1"> are, at least for consistency I would propose the change it back to what it was before

kossebau added inline comments.May 7 2017, 12:59 PM
projectbuilders/makebuilder/makeconfig.ui
122

This string does not match the handling in switch(suCommandNum) for value 2, or?

  • When you edit an UI file, make sure not to forget to increase it's version

This is a valid comment for kxmlgui rc files, but not for Qt ui files :) The version attribute of the <ui> rather specifies the version of the schema used, not of the content.

Too many kinds of ui files for one head, sorry:)

Not sure what the effects of <ui version="4.1"> are, at least for consistency I would propose the change it back to what it was before

No effects, but I reverted it for consistency

projectbuilders/makebuilder/makeconfig.ui
122

sudo just doesn't work as we don't have an interactive terminal here, so I removed it
However, if a user has previously selected sudo, it will be kept as transition code in makejob.cpp still works

kossebau added inline comments.May 7 2017, 1:17 PM
projectbuilders/makebuilder/makeconfig.ui
122

Ah, now I see, okay, so that is for backward compat support.

Please add a comment to the if(suCommandIsDigit) code section to make that clear,
as otherwise some future reader of the code could be confused as well by the handling of those magic numbers.

geetamc updated this revision to Diff 14239.May 7 2017, 4:41 PM

Added a comment explaining the need of "if(suCommandIsDigit)" block.

Added a comment explaining the need of "if(suCommandIsDigit)" block.

Committed

geetamc updated this revision to Diff 14310.May 8 2017, 7:42 PM

-Added a default value for entry "suCommand" in makebuilderconfig.kcfg
-The qstring suCommand is checked for emptiness instead of qstringlist "suCommandWithArg".
(Reason: when an empty qstring is split, the returned qstringlist.isEmpty() is False)

Ignore the previous update, I am making a new review request.