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
Lint Skipped
Unit
Unit Tests Skipped
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 ↗(On Diff #1813)

Coding style:

  • Space after keyword, here and below
  • { on same line, here and below
util/environmentgrouplist.h
26 ↗(On Diff #1813)

Forward decl is enough

util/tests/test_environment.cpp
1 ↗(On Diff #1813)

Add license header

7 ↗(On Diff #1813)

Newline after this line

8 ↗(On Diff #1813)

Please use data-driven tests here.

This is a perfect fit.

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

10 ↗(On Diff #1813)

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
1 ↗(On Diff #1813)

Add license header

apuzio added inline comments.Jan 9 2016, 8:55 PM
util/environmentgrouplist.h
26 ↗(On Diff #1813)

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 ↗(On Diff #1813)

class QProcessEnvironment; here

27 ↗(On Diff #1813)

Remove space

142 ↗(On Diff #1813)

Nitpick: No space after identifier.

Newline before and after.

util/tests/test_environment.cpp
36 ↗(On Diff #1813)

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

... << ProcEnv{
    {key, value},
    {key, value}
}
util/tests/test_environment.h
2 ↗(On Diff #1813)

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 ↗(On Diff #1813)

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 ↗(On Diff #1813)

"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 ↗(On Diff #1813)

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.