Migrate to modern packaging and src layout

[roshii]

• Migrate to src-layout following https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#src-layout to facilitate automatic package discovery and editable installs.
• Create joinmarket namespace distribution package in line with https://packaging.python.org/en/latest/guides/packaging-namespace-packages/
• Migrate to pyproject.toml file for project configuration instead of legacy setup.py

In practice:

• Joinmarket packages are moved to src folder, with their respective test moved to test folder at project root
• Optional dependencies definitions groups allows a granular installation depending on user needs
• Integrated test cannot be run prior to subpackages' respective test, reactor is somehow left unclean by the latter., the former test definitions are therefore moved to a dedicated unified folder.
• A private module is created in jmqtui to be run upon UI update (not tested)

Fixes #1491

[roshii]

Converting to draft as some test are randomly failing. Needs to be investigated further

[AdamISZ]

The reorganization certainly makes sense and would be a step forward, I'm sure.

Create joinmarket namespace distribution package in line with https://packaging.python.org/en/latest/guides/packaging-namespace-packages/

So if I read that right (but, I'm not sure, as I don't exactly understand it) then it still addresses the thing that was "inspiring" me for a rather janky 4 package structure - wanted people to be able to install (jmbase and jmdaemon) or (all 4), plus conceivably certain other combinations. (I'm looking at how they suggest subpackage distributions:

mynamespace-subpackage-a/
    setup.py
    mynamespace/
        subpackage_a/
            __init__.py

mynamespace-subpackage-b/
    setup.py
    mynamespace/
        subpackage_b/
            __init__.py
        module_b.py

Can you explain how this would map to our case? Suppose a person wants just jmbase and jmdaemon and none of the bitcoin or client stuff?

[roshii]

Suppose a person wants just jmbase and jmdaemon and none of the bitcoin or client stuff?

If Alice wants to install jmdeamon today, she will most likely start to clone joinmarket repository or download a release archive, both containing source code for jmbase, jmdeamon et al. packages. She can then install the required packages only and their dependencies, e.g. pip install ./jmbase ./jmdeamon

I did reorganize code with the same approach, with jmbase, jmdeamon et al. packages distributed together with the same version number. In that sense, joinmarket becomes a distribution packages. If Alice wants to install jmdeamon after this refactoring, she can then install the required packages and their dependencies only, running pip install .[deamon]. Note that this will also install any other joinmarket packages to the Python environment although unusable due to missing dependencies.

Splitting jmbase, jmdeamon et al. packages into independent distribution package does not make sense to me considering the current joinmarket release and versioning process and that installing every joinmarket packages, dependencies excluded, is just about copying 1 MB of data, almost negligible.
If, say jmbase, is used as a dependency other Python projects, it should then be split away into an independent distribution package and published to pypi, to be available to any dependent project, joinmarket included. It would then make sense to create a separate distribution package, and it remains straight forward.

That all considered, optimizing installation process is really about optimizing dependencies installation. Since jmbase is required by all other packages its dependencies are installed by default with this refactoring. Optional dependencies installation is triggered with the appropriate identifier, one for each sub-packages plus standard combination (client, demon, gui, etc), see definition at https://github.com/roshii/joinmarket-clientserver/blob/modern-packaging/pyproject.toml#L18. Overall, this should cover most use cases.

I also plan at moving all test related files (e.g. conftest.py) to the test folder to further clarify code structure. Provided tests are passing successfully this should then be ready for review.

[roshii]

I also plan at moving all test related files (e.g. conftest.py) to the test folder to further clarify code structure. Provided tests are passing successfully this should then be ready for review.

The above requires additional modification, to expose scripts as sub-package. These scripts could then be exposed as entry-points, and while it is nice to have, this PR is big enough in itself. Test and script optimization will go for another PR.

[roshii]

Note that some wallet RPC tests are randomly failing on macos such as in https://github.com/roshii/joinmarket-clientserver/actions/runs/4915645134/jobs/8779059629. It looks due invalid cookie, even though cookie validation logic is not changed. Regardless macos test are definitely running slower.

[AdamISZ]

Note that some wallet RPC tests are randomly failing on macos such as in https://github.com/roshii/joinmarket-clientserver/actions/runs/4915645134/jobs/8779059629. It looks due invalid cookie, even though cookie validation logic is not changed. Regardless macos test are definitely running slower.

I'll try to do some testing now on my own (Linux) environment, and see if anything comes up.

[AdamISZ]

First report that the following sequence (run on 9e2dd80 ):

git clone https://github.com/Joinmarket-Org/joinmarket-clientserver
cd joinmarket-clientserver
git clone https://github.com/Joinmarket-Org/miniircd
./install.sh
source jmvenv/bin/activate
cd test; ./run_tests.sh

is successful, with no errors.

[kristapsk]

git clone https://github.com/Joinmarket-Org/miniircd

This is not needed, test/run_tests.sh sets up miniircd, no need to do it manually.

[roshii]

is successful, with no errors.

I have seen no errors on Linux either, only on MacOS, in CI

[roshii]

Rebased on master

[roshii]

I had another occurrence of a test_wallet_rpc case failing on macos, see https://github.com/roshii/joinmarket-clientserver/actions/runs/4972953117/jobs/8898477875, second attempt did succeed

Issue seem related to #1057

[roshii]

We can also see another occurrence of an issue with test_e2e_coinjoin.py getting stuck at https://github.com/JoinMarket-Org/joinmarket-clientserver/actions/runs/4972953255/jobs/8898478043

Issue is unrelated to this refactoring as it was also noticed with legacy setup.py, see #1479 (comment)

[kristapsk]

Needs rebase.

[PulpCattel]

First test: installed successfully with pip3 install -e ./[test]. Test suite passes.

A couple things noticed:

• We should update the doc. At least these two INSTALL.md, TESTING.md, but there might be others.
• If I use an older version of pip (namely 20.3.4), it complains that a setup.py is missing.

ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /joinmarket-clientserver
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

---

Regarding #1484 (comment), I don't have an opinion. The approach taken by this PR seems okay to me.

[PulpCattel]

Do I understand correctly that this means we install test dependencies by default for all users?

[roshii]

test dependencies are only installed with develop flag.
editable installation is always enabled, as per previous behavior.

[roshii]

We should update the doc. At least these two INSTALL.md, TESTING.md, but there might be others.

I will address the above. Thanks.

If I use an older version of pip (namely 20.3.4), it complains that a setup.py is missing.

We can see from pypi statistics that pip 20.3.4 represents less than 4% of total download for June 2023 (I tried for a longer time-frame but it breaks breaks my Google Big Query free quota). Out of those 4% about two thirds are for Python < 3.16, therefore incompatible with joinmarket.

All in all, we have less than 2% of pip user that could be impacted, a marginal joinmarket user base, which could simply update pip.

Unless objections, I will address the above by updating documentation. Thanks.

$ pypinfo -pc --month 2023-06 pip version
Served from cache: False
Data processed: 3.94 GiB
Data billed: 3.94 GiB
Estimated cost: $0.02

| version | percent | download_count |
| ------- | ------- | -------------- |
| 23.1.2  |  82.25% |    101,527,150 |
| 21.3.1  |   6.55% |      8,084,168 |
| 20.3.4  |   3.95% |      4,870,960 |
| 23.0.1  |   1.59% |      1,965,136 |
| 22.3.1  |   1.53% |      1,890,467 |
| 21.2.1  |   1.17% |      1,443,021 |
| 22.2.2  |   0.84% |      1,040,282 |
| 23.0    |   0.77% |        956,088 |
| 21.0.1  |   0.67% |        833,090 |
| 23.1    |   0.67% |        831,404 |
| Total   |         |    123,441,766 |

$ pypinfo -pc --month 2023-06 'pip==20.*' pyversion
Served from cache: False
Data processed: 5.02 GiB
Data billed: 5.02 GiB
Estimated cost: $0.03

| python_version | percent | download_count |
| -------------- | ------- | -------------- |
| 2.7            |  52.86% |      4,394,730 |
| 3.7            |  14.25% |      1,185,002 |
| 3.8            |  11.97% |        995,110 |
| 3.6            |  11.29% |        938,924 |
| 3.5            |   4.17% |        346,687 |
| 3.9            |   3.58% |        297,341 |
| 3.10           |   1.44% |        119,872 |
| 3.11           |   0.39% |         32,629 |
| 3.4            |   0.03% |          2,903 |
| 3.12           |   0.00% |            350 |
| Total          |         |      8,313,548 |

[PulpCattel]

All in all, we have less than 2% of pip user that could be impacted

Personally, I wouldn't trust those numbers much (although of course they give an idea). But more importantly, what's our minimum supported pip version after this PR? (I tested with 20.3.4 just because I already have it on my system)

Whether it's okay or not to flat out drop support for older pip versions IDK, I'll let the others decide. I assume we could find a way to be compatible if we want to.

[kristapsk]

But more importantly, what's our minimum supported pip version after this PR? (I tested with 20.3.4 just because I already have it on my system)

This is important question. If this requires anything newer than 21.3.1 as a minimum, that means we drop Python 3.6 support. Which is ok, but then we could first do that in a separate smaller PR, so that people notice.

[roshii]

After investigation, it comes down to setuptools, v64.0.0 is required after this PR. See pypa/setuptools#2816 for additional info.

And since it requires Python 3.7+, joinmarket would need to match, dropping 3.6 support. Considering the latter is in end-of-life status since end 2021, i don't think it's a big issue.

If no one disagree, I will update requirements accordingly.

[roshii]

Yes, I think you're right, though I don't exactly understand why :)

I did not remember but: testpaths option was before this PR set in setup.cfg, which explains

[AdamISZ]

Merging. From the discussions we've had, there is no disagreement on doing this, but, for sure we need to pay attention to if any small errors crop up from the change.

I'll again ping @openoms @nixbitcoin in an effort to alert people who are doing anything with (shell) scripts running Joinmarket.

[kristapsk]

Post-merge ACK. Will continue to test, but we likely will not do new release too soon, have time for that. And with this being open in parallel with other PRs, that would add a lot of unnecessary rebase work all the time.

[theborakompanioni]

I'll try it as soon as it is merged. Or do you feel more comfortable if I test it before merging? Can surely manage to do this till the weekend. In the very best case, there are, as you mentioned, no changes necessary.

I am getting the following when I try to build Jams dev environment:

76.33 ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /src
76.33 (A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)
76.38 Joinmarket was not installed. Exiting.
------
Dockerfile:17
--------------------
  15 |     RUN git clone "$REPO" . --depth=10 --branch "$REPO_BRANCH" && git checkout "$REPO_REF"
  16 |     
  17 | >>> RUN ./install.sh --docker-install --disable-secp-check --without-qt
  18 |     
  19 |     ENV DATADIR /root/.joinmarket
--------------------
ERROR: failed to solve: process "/bin/sh -c ./install.sh --docker-install --disable-secp-check --without-qt" did not complete successfully: exit code: 1

Notice it runs ./install.sh --docker-install --disable-secp-check --without-qt

Have not tried building the production image yet, but I suppose, it is the same. I have read https://setuptools.pypa.io/en/latest/userguide/development_mode.html, but still a bit lost. @roshii Can you guide me in the right direction?

[kristapsk]

@theborakompanioni Could you try if this change works for you?

diff --git a/install.sh b/install.sh
index 16afd2c..b31e878 100755
--- a/install.sh
+++ b/install.sh
@@ -443,8 +443,11 @@ joinmarket_install ()
         reqs+=',test'
     fi
 
-    if [ "$with_jmvenv" == 1 ]; then pip_command=pip; else pip_command=pip3; fi
-    $pip_command install -e ".[${reqs}]" || return 1
+    if [ "$with_jmvenv" == 1 ]; then
+        pip install -e ".[${reqs}]" || return 1
+    else
+        pip3 install ".[${reqs}]" || return 1
+    fi
 
     if [[ ${with_qt} == "1" ]]; then
         if [[ -d ~/.local/share/icons ]] && [[ -d ~/.local/share/applications ]]; then

[roshii]

See #1484 (comment)

We may need to adapt install script to ensure proper setuptools version is available. It should work in a fresh venv though.

[kristapsk]

@roshi Docker installs doesn't use virtualenv, it's meant to build Docker image for JM. Just also found (forgot about your comment in July) that reason is setuptools version, but pip install --upgrade setuptools is done only in venv_setup(), which is not called with Docker install (as there is no venv to set up).

[kristapsk]

@theborakompanioni Forgot about previous one, try this patch instead:

diff --git a/install.sh b/install.sh
index 16afd2c..da68976 100755
--- a/install.sh
+++ b/install.sh
@@ -176,6 +176,12 @@ check_skip_build ()
     return 1
 }
 
+upgrade_setuptools ()
+{
+    pip install --upgrade pip
+    pip install --upgrade setuptools
+}
+
 venv_setup ()
 {
     if check_skip_build 'jmvenv'; then
@@ -186,8 +192,7 @@ venv_setup ()
     "${python}" -m venv "${jm_source}/jmvenv" || return 1
     # shellcheck source=/dev/null
     source "${jm_source}/jmvenv/bin/activate" || return 1
-    pip install --upgrade pip
-    pip install --upgrade setuptools
+    upgrade_setuptools
     deactivate
 }
 
@@ -621,6 +626,8 @@ main ()
         fi
         # shellcheck source=/dev/null
         source "${jm_root}/bin/activate"
+    else
+        upgrade_setuptools
     fi
     if [[ ${build_local_tor} == "1" ]]; then
         if ! tor_deps_install; then

[theborakompanioni]

@theborakompanioni Forgot about previous one, try this patch instead [...]

That did the trick with debian bullseye! 🙏

Just fyi, on bookworm I get:

0.765 error: externally-managed-environment                                                                                                                                                                                                   
0.765 
0.765 × This environment is externally managed
0.765 ╰─> To install Python packages system-wide, try apt install
0.765     python3-xyz, where xyz is the package you are trying to
0.765     install.
0.765     
0.765     If you wish to install a non-Debian-packaged Python package,
0.765     create a virtual environment using python3 -m venv path/to/venv.
0.765     Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
0.765     sure you have python3-full installed.
0.765     
0.765     If you wish to install a non-Debian packaged Python application,
0.765     it may be easiest to use pipx install xyz, which will manage a
0.765     virtual environment for you. Make sure you have pipx installed.
0.765     
0.765     See /usr/share/doc/python3.11/README.venv for more information.
0.765 
0.765 note: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.
0.765 hint: See PEP 668 for the detailed specification.
------
Dockerfile:17
--------------------
  15 |     RUN git clone "$REPO" . --depth=10 --branch "$REPO_BRANCH" && git checkout "$REPO_REF"
  16 |     
  17 | >>> RUN pip install --upgrade pip
  18 |     RUN pip install --upgrade setuptools
  19 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c pip install --upgrade pip" did not complete successfully: exit code: 1
ERROR: Service 'joinmarket_directory_node' failed to build : Build failed

[kristapsk]

Just fyi, on bookworm I get:

Then this cannot be used longterm, it's new release, we need to support it. This needs some research, but for Jam probably until that my patch could be applied manually for now?

[theborakompanioni]

Then this cannot be used longterm, it's new release, we need to support it. This needs some research, but for Jam probably until that my patch could be applied manually for now?

If there is no other way, then yes. Of course, I would prefer it if it was patched. Is there anything against applying it in master?

edit: I don't know how many users make use of --docker-install.

[kristapsk]

Is there anything against applying it in master?

Not strongly. It's ok if you say so, Jam is the main user of this feature.

[kristapsk]

I will open a PR.

[roshii]

@theborakompanioni setuptools v66 is available from debian packages https://packages.debian.org/bookworm/python3-setuptools

Your image may just need an apt update && apt upgrade python3-setuptools

[theborakompanioni]

Your image may just need an apt update && apt upgrade python3-setuptools

I think that is already being done, no?

[roshii]

I think that is already being done, no?

This is already done indeed. While I suspected an issue with setuptools, bookworm seem to introduce a python environment that does not allow to install system wide package through pip, issue arises with or without forcing setuptools update with pip. As far as I see it, this issue is independent from joinmarket.

You can however amend your Dockerfile as follows to address bookworm issue. Notice the following:

• using python image which does not use this new externally-managed-environment
• removing installation of any python dependencies via apt
• using disable-os-deps-check option from install script

FROM python:3.11-slim-bookworm

RUN apt-get update \
    && apt-get install -qq --no-install-recommends gnupg tini procps vim git iproute2 supervisor \
       # joinmarket dependencies
       curl build-essential automake pkg-config libtool libltdl-dev \
       tor \
       && rm -rf /var/lib/apt/lists/*

ENV REPO https://github.com/JoinMarket-Org/joinmarket-clientserver
ENV REPO_BRANCH master

WORKDIR /src
RUN git clone "$REPO" . --depth=10 --branch "$REPO_BRANCH"

RUN ./install.sh --docker-install --disable-os-deps-check --disable-secp-check --without-qt

[theborakompanioni]

You can however amend your Dockerfile as follows to address bookworm issue. Notice the following:

* using `python` image which does not use this new externally-managed-environment

* removing installation of any python dependencies via `apt`

* using `disable-os-deps-check` option from install script

Works perfectly 👌

In the dev env it is okay, but I am a little reluctant with using disable-os-deps-check in the prod image.. and also somehow I want to try to avoid using python- flavoured images directly. Don't know if that is a counterproductive take, but it feels more pure.

[roshii]

In the dev env it is okay, but I am a little reluctant with using disable-os-deps-check in the prod image..

Since you control how the image is built, I see no issue but I also see your point. Installation script could be amended to cope with (e.g.) python images, verifying presence of executables instead of packages.

and also somehow I want to try to avoid using python- flavoured images directly. Don't know if that is a counterproductive take, but it feels more pure.

It may be difficult with bookworm python package management system, unless using virtual environment. python images are just fine imho, as simple as it can be.