Refactoring and improvement: Config class is now a singleton

[buhtz]

The most important changes were made in common/config.py and common/test/generic.py.

As discussed the class config.Config now acts like a singleton. No matter that there are a lot of possible variants to implement the singleton design patter in Python I choose a pragmatic one, that is IMHO easy to understand for newbies, using less lines and is easy to maintain.

• Create an instance as usual via config.Config()
• If an instance is still there this will raise an Exception.
• Get an existing instance via config.Config.instance()
• This also will raise an Exception if no instance is there.

From an academic point of view this may not fulfill all aspects of the singleton pattern. But it prevents new contributors for misusing it.

I touched a lot of other files to use config.Config.instance() instead of Config(). I also improved the unittests because most of them did create their own Config instance (most of them via generic.TestCaseCfg base class). Now all unittest do destroy there instance if they have created that instance them selfs.

PS: You have to squash before merge Sorry for the 13 commits. This PR was intended to have only one commit but I misused git.

[emtiu]

Hauptsächlich Typos, eine inhaltliche Frage (zur Änderung an sshMaxArg.py)

[aryoda]

I raise concerns that a singleton config does work correctly since I see

• multi-threaded code in the qt GUI (we have no unit tests for the GUI)
• (parallel) async backups where each running backup possibly requires a different config instance.
  At the first glance it looks OK since the config is only used to spawn a new sub process
  but not to reconcile the sub process results into the same config instance but I have not further checked this

See eg. the code when the "Take snapshot" button is clicked:

backintime/qt/app.py

Lines 873 to 875 in b14a98f

	def btnTakeSnapshotClicked(self):
	backintime.takeSnapshotAsync(self.config)
	self.updateTakeSnapshot(True)

and

backintime/common/backintime.py

Lines 48 to 80 in b14a98f

	def takeSnapshotAsync(cfg, checksum = False):
	"""
	Fork a new backintime process with 'backup' command which will
	take a new snapshot in background.
	Args:
	cfg (config.Config): config that should be used
	"""
	cmd = []
	if cfg.ioniceOnUser():
	cmd.extend(('ionice', '-c2', '-n7'))
	cmd.append('backintime')
	if '1' != cfg.currentProfile():
	cmd.extend(('--profile-id', str(cfg.currentProfile())))
	if cfg._LOCAL_CONFIG_PATH is not cfg._DEFAULT_CONFIG_PATH:
	cmd.extend(('--config', cfg._LOCAL_CONFIG_PATH))
	if cfg._LOCAL_DATA_FOLDER is not cfg._DEFAULT_LOCAL_DATA_FOLDER:
	cmd.extend(('--share-path', cfg.DATA_FOLDER_ROOT))
	if logger.DEBUG:
	cmd.append('--debug')
	if checksum:
	cmd.append('--checksum')
	cmd.append('backup')
	# child process need to start its own ssh-agent because otherwise
	# it would be lost without ssh-agent if parent will close
	env = os.environ.copy()
	for i in ('SSH_AUTH_SOCK', 'SSH_AGENT_PID'):
	try:
	del env[i]
	except:
	pass
	subprocess.Popen(cmd, env = env)

Get an existing instance via config.Config.instance()
This also will raise an Exception if no instance is there.

I personally would prefer if the (get)Instance() function does not raise an exception
but creates a new (single) instance internally (stored in a private attribute) if none exists so far.
Exceptions must be tested or handled, this is just extra work.

[buhtz]

I still vote for merge but understand and agree with some of your concerns.

Most of the time the config file is just read. In that case it doesn't matter how often it is opened. In fact in the current situation before that PR dozens of Config instances where created while unittesting in one BIT instances and other BIT instances created via subprocess.Popen() calls.

And that is the intention of that singleton to bring up other problems that might resist in the code.

* multi-threaded code in the qt GUI (we have no unit tests for the GUI)

See above. This isn't a problem. GUI is mutli-threaded by definition. And I assume you mean mutli-processing where backintime is called via subprocess.Popen().

* (parallel) async backups where each running backup possibly requires a different config instance.

I don't see a problem. The term "async" is missleading in the code. It is multi-processing because a complete new backintime instances (and its own python interpreter) is created via Popen().

I see no problem if a sub-process need another config that it should use it. This doesn't effect the singleton. Because the Config instances only exists in the context of one specific backintime instances. When you call subprocess.Popen(['backintime']) that new process does create its own config instance.

See eg. the code when the "Take snapshot" button is clicked:

backintime/qt/app.py

Lines 873 to 875 in b14a98f

	def btnTakeSnapshotClicked(self):
	backintime.takeSnapshotAsync(self.config)
	self.updateTakeSnapshot(True)

I don't see a problem here. That function simply read a bool value from the config instance.
But it points to refactoring.
Handing a whole config object to such a function is bad design. The function should get only that information's that it needs. The point is that other layers of the application and other threads/process should never touch the config instance directly. But currently it is that way. Yes it is easy in the first place just to hand over the whole config. But it is not a good design and cause a lot of trouble. That is one intention of that singleton approach: Reducing the trouble.

That is a big problem of the whole code. To many parts of the code do handling direct the config object. That cause a lot of problems, bad code, etc. Also that BIT does call itself via Popen() isn't a good idea. There are better ways today. But this problem isn't solvable at once. This is a step-by-step thing for the next years.

Get an existing instance via config.Config.instance()
This also will raise an Exception if no instance is there.

I personally would prefer if the (get)Instance() function does not raise an exception but create a new (single) instance internally (stored in a private attribute) if none exists so far.

I strongly disagree because this would undermine the very objective of that PR and the singleton. The config.Config.instance() is not a service to the user/developer. It is a guard who will hit on your fingers if you (or the "old" code you are using) forgot to instantiate a Config object. And Config.__init__() hit on your fingers when you try to instantiate your own Config object.

Exceptions must be tested or handled, this is just extra work.

No. Exceptions like this never need to be tested. That is what exceptions are for. This is not a returning error code. When someone is doing this the whole BIT thing need to stop. That is why there is an exception. A situation like this should never happen.
Having an exceptions points you to the fact that you do something wrong or that some of the code that is involved did something wrong. Somewhere there the loading of a config file doesn't happen. Such an exception is not a usual "error" but a disaster. ;)

[emtiu]

We've agreed to stash this refactoring in its own branch first, while the main branch receives mainly bugfixes.