Remove common package

[drew2a]

This PR is a part of #6505.
It moves all files from the common package to the core package and removes the common package completely.

The new folder structure: https://github.com/drew2a/tribler/tree/refactoring/remove_common_package/src

@drew2a: I've changed Jenkins' jobs regarding the changes as well.
Closes #5645

[kozlovsky]

I have mixed feelings about this change. On the one side, it makes code structure simpler, which is a good thing, as the common package looks a bit ugly. On the other side, it makes answering some previously simple questions harder.

Specifically, we have a question: "what exactly the GUI modules should be able to import from the Core modules" and the previous answer was "nothing". With this change, we should answer this question differently, and it may be "something" (which I don't like) or "everything".

Consider the abstract module x used both in tribler_gui and tribler_core, that was previously placed in tribler_common.

I can imagine the following scenario a year later: some developer is working on a core-related feature. He wants to use some helper function in module x, so he adds an import to it:

from tribler_core.y import helper_function

After this new import is added, the GUI process starts importing the module tribler_core.y as well. This fact may not be obvious to a developer of a core feature. If the module y imports a lot of other modules and libraries, then tribler_gui suddenly starts importing all these libraries as well. The consequences are the following:

• accidentally-imported modules can slightly increase tribler_gui startup time and memory footprint.
• sometimes, a module import has side-effects: the module can create some global object or change system configuration. This can lead to some obscure bugs if the module import is unexpected.

In the current code, this scenario is impossible: a developer cannot add an import of y.helper_function to x, as tribler_common cannot import from tribler_core.

The scenario with abstract modules x and y may look contrived, but I'm almost sure that with this change a year later we will have some unexpected imports of Core modules in GUI.

So, in my opinion, this PR makes the directory structure simpler, and the dependency graph more complex. We can merge it if we agree that for the question "what exactly the GUI modules should be able to import from the Core modules" the answer is "everything".

In this case, requirement files should also be changed: tribler_gui/requirements.txt should include tribler_core/requirements.txt, and the top-level requirements.txt should refer just to tribler_gui/requirements.txt.

[drew2a]

what exactly the GUI modules should be able to import from the Core modules

Agree, that GUI should be able to import any module from the core.
Moreover, it is hard to imagine how the project should be structured to satisfy the answer "something".

After this new import is added, the GUI process starts importing the module tribler_core.y as well.

Agree it is a possibility (as well all consequences that you are mentioned). But this is true for every single import.
So, by merging core and common packages we don't make things worse.

[kozlovsky]

by merging core and common packages we don't make things worse.

The difference is that with the current project structure

• tribler_gui never imports from tribler_core
• tribler_common never imports from tribler_core

So, when tribler_gui imports from tribler_common, it is guaranteed not to import anything from tribler_core.

After this PR, tribler_gui starts importing something from tribler_core, and so it may import anything from tribler_core by accident.

[drew2a]

So, when tribler_gui imports from tribler_common, it is guaranteed not to import anything from tribler_core.

Agree, but do we have any good reason to create and maintain a separate package to satisfy this?

[drew2a]

In this case, requirement files should also be changed: tribler_gui/requirements.txt should include tribler_core/requirements.txt, and the top-level requirements.txt should refer just to tribler_gui/requirements.txt.

Agree, I'll add a commit with the requested changes a bit later :)

[kozlovsky]

Agree, but do we have any good reason to create and maintain a separate package to satisfy this?

Probably not, I just want to be sure that we don't miss any currently important reason why we wanted to prevent imports from Core to GUI.

As we don't have plans to distribute tribler_gui and tribler_core as separate packages, we probably can simplify the logic and allow imports from Core to GUI.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[devos50]

Probably not, I just want to be sure that we don't miss any currently important reason why we wanted to prevent imports from Core to GUI.

The main reason we introduce the intermediate common package was to avoid accidentally importing stuff in the GUI (e.g., libtorrent). These imports can really mess with the logic of the GUI so having an intermediate common package has worked well as a safeguard against this (as @kozlovsky also identified). Additionally, the moment we introduce the tribler_common package, our goal was to improve the structure of our project and avoid duplicate code. Even though we could have directly imported code from the core, we believed it was cleaner to import from an intermediate package. And we also had plans to ship separate packages for the Tribler Core and GUI, since running Tribler without GUI was (and still is) a popular request.

When we directly import modules from the core, it quickly becomes challenging to track which parts we are importing in the GUI. So I don’t think we can, nor should answer the question: “which parts of the core should be imported by the GUI”. It can be everything. And the import chain can escalate and pollute the module namespace of the GUI. We had these kinds of problems a few times in the past so I liked the fact that the common package helped to prevent this.

If I have to give my opinion, I would vote to keep it. But I also see the arguments in favour of removing it (simplicity and the fact that we never pursued the idea of shipping packages in Tribler). We all discussed and agreed on #6505 so if this is part of the deal, I’ll accept it :)

Minor note: this PR might affect the relevance of #5645.

[drew2a]

@devos50

The main reason we introduce the intermediate common package was to avoid accidentally importing stuff in the GUI (e.g., libtorrent). These imports can really mess with the logic of the GUI so having an intermediate common package has worked well as a safeguard against this (as @kozlovsky also identified).

The reason is clear, but the presence of the #5645 shows that the intermediate common package doesn't protect us against "accidentally importing stuff in the GUI".

Additionally, the moment we introduce the tribler_common package, our goal was to improve the structure of our project and avoid duplicate code.

This is also clear, but the presence of the common package could even increase code duplication :) (as it requires additional effort to maintain commonly used code in the separate repo). So, from my experience, this "additional effort" could lead to creating the same function/procedures in separate packages instead of deduplication.

Imagine the situation in which a developer creates a new feature in the GUI module.
The feature requires some function (to simplification let's say it is a verify_path function).
If we have common and core packages, then this developer should go to the common package first, and search the function. If there is no such function, the developer should go to the core and again, search the function (2 searches in total).
If we have only the core package, the search count decreases to 1.

The difference in just 1 search attempt seems to be insignificant, but I believe it could lead to the situation in which the developer could end his search at the first attempt and the deduplication will not be implemented properly.

And we also had plans to ship separate packages for the Tribler Core and GUI, since running Tribler without GUI was (and still is) a popular request.

The merging core and common doesn't interfere with that goal (as far as I know).

[drew2a]

If the import of libtorrent is the main problem -- we could add an explicit check to the PR to prevent this (anyway the common package doesn't protect us from this).