This differential is discarded in favour of D14870 and should be removed.
Extends DebugJob class including launch configuration environment variables.
BUG: 322477
Details
Details
- Reviewers
Kebianizao - Group Reviewers
KDevelop
Diff Detail
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
| 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. | |
Comment Actions
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.
| debugger/debugsession.cpp | ||
|---|---|---|
| 98 | If I'm right, I'm modifying env variable later: | |
| debugger/pdblauncher.cpp | ||
| 124 | ACK. | |
| 130 | ACK. | |
| 134 | I well may wrong, but what I can tell is that: for(const auto &i : environment)
{
job->m_environment << environment[i];
}I don't know if this is what you were thinking. | |
Comment Actions
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. | |
Comment Actions
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. | |
Comment Actions
I have infinite patience in watching other people do the work, so by all means, go ahead! :-)
Thanks!
| debugger/pdblauncher.cpp | ||
|---|---|---|
| 134 |
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. | |
Comment Actions
Updated commit according to commenters feedback.
Keeping original approach, still improvable,
Comment Actions
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 ...?
Comment Actions
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.