Bug 350183 - Support of $VARIABLE in environment settings
ClosedPublic

Authored by kfunk on Jan 9 2016, 8:02 PM.

Details

Summary

Added the possibility to use $VARIABLE in environment settings.
After some discussion it was concluded, that we won't support defining a variable with other user defined variable. Only supported scenario is defining a variable with a system variable.

Test Plan

Tested with the added unit test and in practice.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apuzio updated this revision to Diff 1813.Jan 9 2016, 8:02 PM
apuzio retitled this revision from to Bug 350183 - Support of $VARIABLE in environment settings.
apuzio updated this object.
apuzio edited the test plan for this revision. (Show Details)
apuzio added a reviewer: kfunk.
apuzio set the repository for this revision to R33 KDevPlatform.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 9 2016, 8:02 PM
kfunk edited edge metadata.Jan 9 2016, 8:16 PM

Looks good already, just a few issues.

Thanks

util/environmentgrouplist.cpp
210

Coding style:

  • Space after keyword, here and below
  • { on same line, here and below
util/environmentgrouplist.h
26

Forward decl is enough

util/tests/test_environment.cpp
2

Add license header

8

Newline after this line

9

Please use data-driven tests here.

This is a perfect fit.

See: http://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

11

I'd suggest to build your own QProcessEnvironment (cf. QProcessEnvironment::insert, etc.), and pre-fill with values for PATH/HOME.

Then testing code gets easier, and you can use plain strings.

util/tests/test_environment.h
2

Add license header

apuzio added inline comments.Jan 9 2016, 8:55 PM
util/environmentgrouplist.h
26

What do you mean? Removing this line brakes the build with error "QProcessEnvironment not defined"

apuzio updated this revision to Diff 1821.Jan 9 2016, 8:56 PM
apuzio edited edge metadata.
apuzio updated this revision to Diff 1823.Jan 9 2016, 8:58 PM
apuzio updated this revision to Diff 1824.Jan 9 2016, 9:01 PM
apuzio marked 6 inline comments as done.
kfunk added inline comments.Jan 9 2016, 9:23 PM
util/environmentgrouplist.h
26

class QProcessEnvironment; here

27

Remove space

142

Nitpick: No space after identifier.

Newline before and after.

util/tests/test_environment.cpp
36

Please avoid overlong lines, hard to read in the diff viewer already.

... << ProcEnv{
    {key, value},
    {key, value}
}
util/tests/test_environment.h
2

Nitpick: Use a nicer formatting for the license :)

/*
 * DESC
 *
 * Copyright NAME
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU Library General Public License as
 * published by the Free Software Foundation; either version 2 of the
 * License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public
 * License along with this program; if not, write to the
 * Free Software Foundation, Inc.,
 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
 */
23

Newline before

apuzio updated this revision to Diff 1826.Jan 9 2016, 9:51 PM
apuzio marked 8 inline comments as done.
kfunk added a comment.Jan 9 2016, 9:54 PM

Looks good to me!

Will test a bit and then push. But I think your GCI task is done. Thanks a lot for your patience and patch, great work!

kfunk, Thank you for all help. I learned a lot while doing this task.

kfunk commandeered this revision.Jan 9 2016, 10:43 PM
kfunk edited reviewers, added: apuzio; removed: kfunk.
kfunk updated this revision to Diff 1829.Jan 9 2016, 10:43 PM
kfunk updated this object.

Cleanup

kfunk added a comment.Jan 9 2016, 10:44 PM

On hold for now.

This doesn't work as intended for the major use-case: Native app jobs. They're not even based on OutputExecuteJob. And I have no idea why; it would make a lot of sense as far as I understand...

kfunk added a comment.Jan 9 2016, 11:41 PM
In D770#14709, @kfunk wrote:

On hold for now.

This doesn't work as intended for the major use-case: Native app jobs. They're not even based on OutputExecuteJob. And I have no idea why; it would make a lot of sense as far as I understand...

Done. See here: https://phabricator.kde.org/D773

apuzio added inline comments.Jan 10 2016, 5:37 PM
util/tests/test_environment.cpp
77

"env" and "environment" isn't the best naming.
I would suggest:
env -> userEnv
environment -> sysEnv

kfunk added inline comments.Jan 10 2016, 6:26 PM
util/tests/test_environment.cpp
77

Will fix. Thanks

I accidentally pushed a WIP patch anway (thus all the qDebug calls...). arc fail.

This revision was automatically updated to reflect the committed changes.