#1179 Config save bug - code path dependent

[Andrew Cottrell]

Sorry for this huge report , but it gets a little bit complex and I have included a potential untested solution. See https://forums.codeblocks.org/index.php/topic,24800.0.html for forum post

I am working on getting my code for ticket 1114 ready for a patch to be submitted as allot of changes have occurred since August 2021.

I am ironing out some config bugs and want to check if my assumptions and findings are right or wrong. If they are wrong please point me on where I went wrong so I can get to the final solution faster.

I have found the following:
1) In the void CompilerFactory::SaveSettings() calls the following:
Code

void CompilerFactory::SaveSettings()
{
// clear old keys before saving
Manager::Get()->GetConfigManager(_T("compiler"))->DeleteSubPath(_T("/sets"));
Manager::Get()->GetConfigManager(_T("compiler"))->DeleteSubPath(_T("/user_sets"));

for (size_t i = 0; i < Compilers.GetCount(); ++i)
{
    wxString baseKey = Compilers[i]->GetParentID().IsEmpty() ? _T("/sets") : _T("/user_sets");
    Compilers[i]->SaveSettings(baseKey);

Which deletes all of the compilers.. Ouch
And then at first glance of the code saves the settings, BUT the Compiler::SaveSettings(const wxString& baseKey) code only saves the changes since the call to the Compiler::MirrorCurrentSettings() that occurs on loading.... another Ouch but even more worring

Snippet of code from Compiler::SaveSettings(const wxString& baseKey) :
Code

if (m_Mirror.CompilerOptions_ != m_CompilerOptions)
{
    wxString key = GetStringFromArray(m_CompilerOptions);
    cfg->Write(tmp + _T("/compiler_options"), key, false);
}

2) The Compiler::SaveSettings(...) does not flush the file to disk at the end. Another minor change.

My query is:
Does anyone know why the CompilerFactory::SaveSettings() removed the keys and then the Compiler::SaveSetting() only saves the changes, so that if this code is executed then you could end up with a bad config file. This is occurring with me for the mods I am making.

Potential Conclusion:
This could be what is causing the corrupted (aka bad) config files that are reported a few times are year as it depends on the code path as to if the problem outlined above occurs.

Possible First Solution:
a) Change the CompilerFactory::SaveSettings() to be:
Code

void CompilerFactory::SaveSettings()
{
for (size_t i = 0; i < Compilers.GetCount(); ++i)
{
if (Compilers[i]->IsValid())
{
// Only save valid compiler configuration settings
wxString baseKey = Compilers[i]->GetParentID().IsEmpty() ? _T("/sets") : _T("/user_sets");
Compilers[i]->SaveSettings(baseKey);

        CodeBlocksEvent event(cbEVT_COMPILER_SETTINGS_CHANGED);
        event.SetString(Compilers[i]->GetID());
        event.SetInt(static_cast<int>(i));
        event.SetClientData(static_cast<void*>(Compilers[i]));
        Manager::Get()->ProcessEvent(event);
    }
}

}

 The delete keys are moved into the SaveSettings(...) as it already deleted the old style keys
 This way only valid compiler data is saved instead of partial data for invalid compilers

c) Update the Compiler::SaveSettings(const wxString& baseKey) to do the following:
1) Add the DeleteSubPath and the first comment line (the rest are exisiting lines)
Code

// delete old keys before saving
tmp.Printf(_T("%s/%s"), baseKey.c_str(), m_ID.c_str());
cfg->DeleteSubPath(tmp);

cfg->Write(tmp + _T("/name"),   m_Name);
cfg->Write(tmp + _T("/parent"), m_ParentID, true);

2) Change the code to save all of the config data instead of checking if it has changed
3) Add a cfg->Flush(); at the end of the function.

d) Remove the MirrorCurrentSettings()

With the changes above the config file still works and is allot bigger than before.

When I get time I will supply two patches:
1) First patch for the changes above
2) Tidy up the code once the first patch has been applied and it is deemed "stable"/working. This could be done against a new ticket "Remove redundant mirror variables in the compiler class"

Once the first patch has been applied I can then submit a patch for ticket 1114.

[Andrew Cottrell]

As per the description above. Attached is the first patch.

Once this has been applied I will work on the second patch and then ticket 1114.

[Miguel Gimenez]

I do not think removing the call to MirrorCurrentSettings() is a good idea, it is there for a reason.

Also, a compiler may be invalid but its contents must be saved anyway (lack of validity may be a temporary failure, or just a typo).

[Andrew Cottrell]

I have been working on the clangd_client code base and incorporating additional/missing plugins into my installer so it has taken a while to get back to this.

Please do some testing with the changes and see how much extra stuff is it the new config and therefore the conclusion will be that the existing code is buggy and not storing info into the config that is expected from the code.
You could try with just commenting out the MirrorCurrentSettings() and see what happens with the config file as a QAD test. The next test is to add in the flush. And then....