Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement Request: Restructure main Tribler folders #6505

Closed
drew2a opened this issue Oct 27, 2021 · 13 comments
Closed

Improvement Request: Restructure main Tribler folders #6505

drew2a opened this issue Oct 27, 2021 · 13 comments
Assignees
Milestone

Comments

@drew2a
Copy link
Contributor

drew2a commented Oct 27, 2021

The current structure of Tribler folders is following:

src/
    pyipv8/
    tribler-core/
        tribler_core/
    tribler-gui/
        tribler_gui/
    tribler-common/
        tribler_common/

The directory structure was created like this to facilitate PyPI shipping of those 3 directories. (@qstokkink)
But this PyPI packaging never happened. We can restructure it again but that'll require changing other scripts and Jenkins jobs (PATH exports). My opinion at this moment is to keep it as it is for now and revisit it later. (@xoriole)

How to restructure folders

Below I placed suggestions from @kozlovsky:

I don't think it is important to have separate directories for core/gui/common. But it does not hurt much to have them separated, either. As @xoriole said, refactoring may break some scripts. So I also think it may be better to leave it as-is for now.
Also, if we are going to change the directory structure, we need to discuss possible alternatives. I see the following alternatives:

  1. Tribler as a package with three subpackages
src/
    tribler/
        __init__.py
        core/
        gui/
        common/
  1. Three different packages
src/
    tribler_core/
    tribler_gui/
    tribler_common/
  1. single package one level deeper:
src/
    pyipv8/
        ipv8/
            __init__.py
    tribler/
        tribler/
            __init__.py
            core/
            gui/
            common/

The third option is as deep as the current approach. The only difference is that three subdirectories, tribler-core , tribler-common, and tribler-gui are merged together. So it does not give much simplification of the directory structure.
But with approaches 1 & 2, it is not clear where to put the pyipv8 directory. Note that this directory is not a package; a package ipv8 is placed inside it. So we cannot just add src to PYTHONPATH and import everything from it.

IPv8

But where to place pyipv8 then? It does not feel correct for me to have pyipv8, which is not a package, on the same level as a new package tribler (@kozlovsky)

I have no strong opinion on this, I just wanted to say that you can also choose to handle IPv8 as an external dependency. It is on PyPI: https://pypi.org/project/pyipv8/ (@qstokkink)

@drew2a
Copy link
Contributor Author

drew2a commented Oct 27, 2021

Criticism of this proposal by @ichorid

As @xoriole pointed out correctly, the current directory structure adheres to standards of PyPi packaging. PyPi packaging guidelines is the closest thing Python has to a "standard" way of organizing a project's structure. This double-nesting is the result of Python not allowing to use - in module names. So, professional Python coders reading our codebase expect it to include such a structure (even though it could look non-intuitive to people coming from other language backgrounds).
If we don't want to surprise experienced Python coders, we should keep the current structure.

@synctext
Copy link
Member

synctext commented Jan 12, 2022

Disclaimer: opinion of former coder.
This is an issue close to my heart. It got uglier in the past decade 🤣. Basically, its difficult to navigate the source code tree. What I would propose (without having to suffer the consequences daily):

@drew2a
Copy link
Contributor Author

drew2a commented Mar 7, 2022

The next step will be to align the structure to the following:

build/

tribler/
    core/
    gui/

scripts/
    seedbox/
    experiments/
        popularity_community/
        tunnel_community/
    crawler/
    exit_node/

@qstokkink
Copy link
Contributor

It's probably best to leave the core and GUI classes separated as much as possible. Qt5 and asyncio don't really work well together (though there are libraries, like qasync, to deal with this). Calling async stuff from Qt5 can lead to weird behavior.

I agree with the design of core/ and gui/, but flattening the directory structure more than that will likely lead to heartache.

@kozlovsky
Copy link
Contributor

Thoughts on the proposed structure:

  1. Are core/ and gui/ some wrapping directories (renamed from tribler-core and tribler-gui) or packages (renamed from tribler_core and tribler_gui)? If they are wrapping directories, we probably don't need them. In they are packages, we'll need to write imports like from core import something, which may lead to conflicts with some non-Tribler-related packages core and gui. It is better to have tribler in the package name and name them tribler_core and tribler_gui.

  2. When someone clones the Tribler repo, the top-level directory is usually named tribler. So, we will have two nested tribler directories with the proposed structure, and then sentences like "put something in tribler directory" become ambiguous. In that sense, the src name for the directory looks less confusing. A counter-argument may be that scripts are also sources, and if we have an src folder, it is strange that we place scripts outside of it. But src can mean sources for Tribler binary, and then it is logical that scripts are placed outside of src.

What about the following structure:

build/

src/
    tribler_core/
    tribler_gui/
    run_tribler.py

scripts/
    seedbox/
    experiments/
        popularity_community/
        tunnel_community/
    crawler/
    exit_node/

@drew2a
Copy link
Contributor Author

drew2a commented Mar 8, 2022

@kozlovsky thank you for the proposals, below are my comments:

Are core/ and gui/ some wrapping directories (renamed from tribler-core and tribler-gui) or packages (renamed from tribler_core and tribler_gui)? If they are wrapping directories, we probably don't need them. In they are packages, we'll need to write imports like from core import something, which may lead to conflicts with some non-Tribler-related packages core and gui. It is better to have tribler in the package name and name them tribler_core and tribler_gui.

They are packages.
Conflicts with non-Tribler-related packages core and GUI are very (very) unlikely possible. So, it seems that the names core and gui are safe enough.
But I have no strong opinion on removing the prefix tribler from core and gui. The prefix is unnecessary and for me, it looks ugly.

When someone clones the Tribler repo, the top-level directory is usually named tribler. So, we will have two nested tribler directories with the proposed structure, and then sentences like "put something in tribler directory" become ambiguous. In that sense, the src name for the directory looks less confusing. A counter-argument may be that scripts are also sources, and if we have an src folder, it is strange that we place scripts outside of it. But src can mean sources for Tribler binary, and then it is logical that scripts are placed outside of src.

Sentences like "put something in tribler directory" will be ambiguous in any case, unless we explicitly define the term "tribler directory", so src doesn't help with it.

But src can mean sources for Tribler binary

This assumption is not obvious, moreover, for me personally, it is a bit counterintuitive.

@drew2a
Copy link
Contributor Author

drew2a commented Mar 8, 2022

@qstokkink

I agree with the design of core/ and gui/, but flattening the directory structure more than that will likely lead to heartache.

Does this mean that you agree with the proposed folder's design and disagree with the further fluttering (e.g. merging core and gui)?

Or does this mean that this is ok (1):

build/

tribler/
    core/
        tribler-core/
            *.py
    gui/
        tribler-gui/
            *.py

and this is not ok (2):

build/

tribler/
    core/
        *.py
    gui/
        *.py

@qstokkink
Copy link
Contributor

@drew2a that is exactly right. Both (1) and (2) seem fine with me. I just think the following is bad:

build/
tribler/
    *.py
scripts/
   ...

@drew2a
Copy link
Contributor Author

drew2a commented Mar 8, 2022

@qstokkink thank you for the confirmation, now it is clear :)
I agree with you.
As far as I know, this approach also supports both @devos50 and @kozlovsky

@kozlovsky
Copy link
Contributor

For me, the desire to name directories tribler/core/ and tribler/gui/ looks purely aesthetical. It can lead to some minor problems. They do not look critical but can happen anyway. src/tribler_core/ and src/tribler_gui/ names keep the same directory structure, just with different names, and prevent these potential problems.

  1. Some developers can use a pretty popular core library for emulating networks and have Tribler cloned as a local repo. Then imports like from core import something can be ambiguous for these users.

  2. At least previously, Tribler was run as a service with user named tribler. It may be inconvenient to have tribler home directory, then tribler repo directory, then tribler internal directory that contains actual sources, and commands like cd tribler become less obvious in this setup. It is better to avoid nested directories with the same name, if possible.

Regarding the src name for a directory, I think it is common practice. For example:

“src” is generally a universally recognized location for source code, making it easier for others to quickly navigate the contents of your package. (https://py-pkgs.org/04-package-structure.html)

Of course, the usual Python guidelines are for packages, and Tribler is not a package, so these guidelines are not directly applicable. Anyway, it may be easier for external developers if the Tribler directory structure resembles a standard Python project's structure.

But the above-mentioned problems, while real, are probably not that critical, so if someone insists that we should switch to tribler/core/ and tribler/gui/ I'm ok with that.

@kozlovsky
Copy link
Contributor

After additional discussion with @drew2a, we come up with the following directory structure:

build/
src/
    scripts/
    tribler/
        __init__.py
        core/
        gui/
    run_tribler.py
requirements.txt
core_requirements.txt

With this approach, core and gui became sub-packages of the tribler package. Is it dangerous? I think no. They are still separated because we do not mix Qt and Asyncio event loops, but we can do from tribler.core import something or even use relative imports. This approach leads to the most concise imports.

Why the src directory? We need to add some directory to PYTHONPATH, and without src, it would be the root project directory that contains unrelated stuff. It is usually considered a good approach to not add to PYTHONPATH directories with non-related potentially importable content. Also, the src directory contains not only the tribler package but the run_tribler.py script as well.

Where to place the scripts directory? If src means "stuff that packed and distributed to the user as a Tribler binary", then scripts should be placed outside of src. If src means "any Python code", it is more logical to put it inside src. I'm 50/50 on that.

@synctext @qstokkink @devos50 @xoriole, what do you think? Can we make tribler a package with two sub-packages and use imports like from tribler.core.something import something?

@qstokkink
Copy link
Contributor

@kozlovsky Firstly, I see no objections to this proposal. Secondly, intuitively, I'd put the scripts next to the src directory, instead of in it. This is mostly because I consider a script to be (1) independent code that (2) imports from Tribler, (3) is not imported by Tribler, and (4) is not shipped to a user. However, if one of those four conditions does not hold (like you suggested might be the case), you probably have to put the scripts directory inside of the src directory.

@drew2a
Copy link
Contributor Author

drew2a commented Mar 9, 2022

The next steps will be:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants