Remove editable installs

[roshii]

Remove editable install option to allow tests to run in a fresh venv.

Issue looks related to pypa/setuptools#3504
It should be noted that issue may not arise in CI due to venv cache being restored. This is a quick fix to allow tests to run, a structural fix could be applied by moving away from legacy setup.py to a single pyproject.toml defining each and every packages .

[kristapsk]

This breaks --develop setting of install.sh (which is currently always on). Historically it was off by default for users, but for developers it is very useful feature.

joinmarket-clientserver/install.sh

Line 517 in 01dc08a

--develop code remains editable in place (currently always enabled)

[kristapsk]

Btw, I have setuptools 67.2.0 installed on my development machine and haven't noticed any problems with running JM itself or test suite.

[roshii]

Btw, I have setuptools 67.2.0 installed on my development machine and haven't noticed any problems with running JM itself or test suite.

Thanks. I cannot confirm my development machine is all set as it should but GitHub Actions should definitely run on any fork. I will investigate further.

[kristapsk]

Was trying this approach specifically for CI, by removing -e with sed before setting up virtualenv, but for some reason all runs except python 3.7 don't get past 98% of test suite with "The job running on runner GitHub Actions 8 has exceeded the maximum execution time of 360 minutes" error. Not sure what is the reason. https://github.com/kristapsk/joinmarket-clientserver/actions/runs/4823601007

[roshii]

I assume there is something wrong with the way subpackages are define, Python 3.7 may be running 'legacy' install logic, only assumptions so far...

I will try to switch it to a single pyproject.toml

[kristapsk]

Now I also have this problem on my local machine.

____________________________________________________________________ ERROR collecting jmbitcoin/test/test_ecc_signing.py _____________________________________________________________________
ImportError while importing test module '/home/user/git/joinmarket-clientserver/jmbitcoin/test/test_ecc_signing.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.9/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
jmbitcoin/test/test_ecc_signing.py:7: in <module>
    from jmbase import bintohex
E   ImportError: cannot import name 'bintohex' from 'jmbase' (unknown location)
...

[roshii]

Now I also have this problem on my local machine.

I'm not alone :)
Working on #1484 for a structural fix. I have yet to address a few tests issues but the idea is there.

[kristapsk]

Note that there is error only when running ./test/run_tests.sh. But not when I activate virtualenv and run, for example, ./scripts/wallet-tool.py.

Working on #1484 for a structural fix.

That looks like a quite big change, may take a lot of time to review and what is rationale, as we would likely prefer fast fix, at least short term. I'm not saying it's right or wrong approach long term, need then to do more reasearch on Python packaging, what are pros and cons there.

[roshii]

Was trying this approach specifically for CI, by removing -e with sed before setting up virtualenv, but for some reason all runs except python 3.7 don't get past 98% of test suite with "The job running on runner GitHub Actions 8 has exceeded the maximum execution time of 360 minutes" error. Not sure what is the reason. https://github.com/kristapsk/joinmarket-clientserver/actions/runs/4823601007

I'm experiencing similar issue on https://github.com/roshii/joinmarket-clientserver/actions/runs/4874741102/attempts/2
There seem to be issue with test_e2e_coinjoin.py which apparently get stuck on some edge cases

[roshii]

After rebasing to master, CI leads to the very same error as your branch: we have Python 3.7 tests passing but other Python version failing with:

• test_wallet_rpc.py failing or in error
• test_e2e_coinjoin.py not responding

We can clearly see that there is something wrong with tests at the moment:

• test suite does not run on a fresh environment with editable installs
• test suite fails with non-editable install
• test suite fails in some edge cases with modern packaging approach and editable installs.

When skipping test_e2e_coinjoin.py, we can see errors log for test_wallet_rpc.py such as https://github.com/roshii/joinmarket-clientserver/actions/runs/4885165879/jobs/8718872833#step:10:203, error consistent with what I have noticed with the modern packaging approach

I am now trying to understand test_wallet_rpc.py errors relating to reactor being left unclean, will focus on non responding test next. Fixing these will allow CI properly running, a structural fix of editable install should come after imho.

[roshii]

Failing test_wallet_rpc.py tests

These were due to werkzeug, this is solved.

Joinmarket packages ImportError

For the following, not responding test_e2e_coinjoin.py is not considered and skipped if Python >= 3.8

With a.m. test fixed, I had CI running, successfully (excluding a non reproducible error such as in https://github.com/roshii/joinmarket-clientserver/actions/runs/4892957670/jobs/8735330345)
However, re-running the very same test suite, which triggers venv cache to load, leads to the original ImportError, such as in https://github.com/roshii/joinmarket-clientserver/actions/runs/4893115460/jobs/8740254260. Invalidating venv cache does again trigger test suite to succeed.

To resume, and as far as I can tell:

• test suite does not run on a fresh environment with editable installs
• test suite does not run with cached venv with non editable installs
• test suite fails in some edge cases with non editable installs and fresh venv
• test suite fails in some edge cases with modern packaging approach and editable installs

A quick fix could be applied by removing the cache-venv step. I don't think it make sense to solve it without a more structural approach on packaging.

[kristapsk]

Big thanks for trying to figure this out!

[roshii]

Closing PR with fixes implemented by #1490

[kristapsk]

We still have editable install problem for running tests locally, but for that separate issue can be opened.