Inject default environment group variables into debug process environment.
AbandonedPublic

Authored by kossebau on Aug 9 2018, 1:31 PM.

Details

Reviewers
Kebianizao
Group Reviewers
KDevelop
Summary

This differential is discarded in favour of D14870 and should be removed.

Extends DebugJob class including launch configuration environment variables.
BUG: 322477

Diff Detail

Repository
R53 KDevelop: Python Support
Branch
arcpatch-D14710
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1886
Build 1904: arc lint + arc unit
Kebianizao created this revision.Aug 9 2018, 1:31 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptAug 9 2018, 1:31 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
Kebianizao requested review of this revision.Aug 9 2018, 1:31 PM
Kebianizao edited the summary of this revision. (Show Details)Aug 9 2018, 1:36 PM
apol added a subscriber: apol.Aug 9 2018, 4:46 PM
apol added inline comments.
debugger/debugsession.cpp
98

const

debugger/pdblauncher.cpp
124

We usually don't translate warnings that won't show on the UI.

130

const

vkorneev added inline comments.
debugger/pdblauncher.cpp
134

Sorry, I'm not actually the reviewer, but may I ask you, why don't use

for(const auto &i: environment) {

here? Just interested if I miss something.

Hi All:
Sorry for the delay. I've been AFK for some days and I afterwards I needed to resettle my development environment. I think I'm almost ready.
@apol Thanks a lot for revision. I'll answer your comments in a moment.
@vkorneev I'll answer your comment in a moments.
After I had the differential reviewed I noticed I had a slightly different version, where environment variables are injected at a different point. Unfortunately, I don't have access to that version so I can't provide it just now. If you want I may send it in two weeks (I know, it's quite a lot, don't ask) or we can proceed with a final solution in this differential.

Kebianizao added inline comments.Aug 15 2018, 8:34 PM
debugger/debugsession.cpp
98

If I'm right, I'm modifying env variable later:
env.insert(key, value);
Therefore I'm not allowed to mark it as const, right?

debugger/pdblauncher.cpp
124

ACK.

130

ACK.

134

I well may wrong, but what I can tell is that:
environment is a QMap<QString,QString>
when I do:
for(const auto &i: environment), i type is QString so I should have to write this chunk as:

for(const auto &i : environment)
{
    job->m_environment << environment[i];
}

I don't know if this is what you were thinking.
Now I got to this, maybe this is a good idea. I can submit the patch with this change if you like.
Reference: https://stackoverflow.com/questions/8517853/iterating-over-a-qmap-with-for#8529237

I'm updating the patch. I will send it as soon as I have it.

brauch added a subscriber: brauch.Aug 15 2018, 8:43 PM

Thanks for the patch!

Sorry, it has been years since I looked at the debugger code. Why do you need to set the environment twice, once on the job and once on the process? Otherwise, this looks fine.

debugger/debugsession.cpp
98

No, you are right, it's fine like you have it

debugger/pdblauncher.cpp
134

Either way is fine imo, I would maybe just initialize the iterator inline with auto:

for (auto i = environment.begin() ...)

It really doesn't matter much though.

Thanks for the patch!

Sorry, it has been years since I looked at the debugger code. Why do you need to set the environment twice, once on the job and once on the process? Otherwise, this looks fine.

As I review the code, I slowly remember what the "improved" version did. I think what it does is first storing the environment profile name in a DebugSession class member and then merging the default process environment with the profile environment variables in the DebugSession start function. If you have the patience I'll fix this differential following the initial approach and afterwards I'll consolidate the simplified environment handling approach in a new differential.

debugger/debugsession.cpp
98

OK.

debugger/pdblauncher.cpp
134

Sorry I meant:

for(const auto &i : environment)
{
         job->m_environment << i + "=" + environment[i];
}

But anyway, I'll stick with what @brauch suggests.

I have infinite patience in watching other people do the work, so by all means, go ahead! :-)

Thanks!

vkorneev added inline comments.Aug 15 2018, 9:26 PM
debugger/pdblauncher.cpp
134

But anyway, I'll stick with what @brauch suggests.

Sure. The version suggested by @brauch is the best one here. The one I was thinking about is wrong, because dereferencing QMap`s const_iterator returns value, not the pair or key (I didn't notice that).

@brauch's version is right and the scope of the iterator is more limited than in your first version, so it's better. Although, if you're not modifying anything, maybe you can use cbegin() and cend() instead of begin() and end(), but as @brauch said it really doesn't matter much.

Kebianizao updated this revision to Diff 39814.Aug 15 2018, 9:34 PM

Updated commit according to commenters feedback.
Keeping original approach, still improvable,

I'm actually fine with this approach, I just don't understand why the environment is set once on the process and once on the job. Why is this necessary, or do I misunderstand what it's doing ...?

I'm actually fine with this approach, I just don't understand why the environment is set once on the process and once on the job. Why is this necessary, or do I misunderstand what it's doing ...?

I'm reviewing this right now. I don't have a reasonable answer for your question. As the patch was my first working version, I think I only managed to get the environmentProfiles variables at PdbLauncher::start not noticing that I could get the variables from a specific environment profile anywhere like this:

const KDevelop::EnvironmentProfileList environmentProfiles(KSharedConfig::openConfig());

Later I though that I could just simply store the environment profile name at PdbLauncher::start, include it in the DebugJob object and tweak the debugee process environment just before the debug process is created at DebugSession::start.
Final patch will arrive in short.

This differential is (hopefully) superseded by https://phabricator.kde.org/D14870

Kebianizao edited the summary of this revision. (Show Details)Aug 16 2018, 11:50 AM
kossebau commandeered this revision.Sep 23 2018, 11:52 AM
kossebau abandoned this revision.
kossebau added a reviewer: Kebianizao.

Closing this, as patch has been abandoned in favour of (landed) D14870