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

Packaging tinydecred #35

Closed
teknico opened this issue Dec 23, 2019 · 22 comments · Fixed by #59
Closed

Packaging tinydecred #35

teknico opened this issue Dec 23, 2019 · 22 comments · Fixed by #59
Assignees

Comments

@teknico
Copy link
Collaborator

teknico commented Dec 23, 2019

tinydecred is already useful enough that people ask for an easy, standard way to install and use it (see #34). I'd like to explore several issues related to such packaging.

How to package it

Without going into many details, Python's packaging story has been a confusing mess and one of its weak points. (I've been using and teaching Python for 20 years now, so I've seen a thing or two).

Thankfully in recent years the approval of PEP 517 and PEP 518 opened the way to a new wave of better and simpler tools.

The most promising, complete, and most used one is Poetry. In addition to building and publishing packages it features a complete dependency tracker like the ones in Go and Rust, allowing deterministic builds and the management of the whole project lifecycle.

What to package

The tinydecred repo includes both the library proper and a wallet GUI app which adds Qt and other dependencies. It's advisable to create two separate packages for the two sets of code.

The separation will help reasoning about the library's API and its versioning, where a given version of the GUI app will depend on a given version of the library.

The GUI app code could also be split off into a separate repo (named tinywallet or something?). Building multiple Python packages off a single repo is somewhat less clean, even though it can work.

Publishing to PyPI

In order to publish to the PyPI public repo we need an account, I don't know if the project has already one available.

Who's going to do it

I will do the work once someone creates a repo for the GUI app. It will involve moving files around so more gigantic PRs, I'm afraid.

@teknico
Copy link
Collaborator Author

teknico commented Dec 24, 2019

Here's an example of how to set up an efficient CI on Github Actions with Poetry and other tools we already use (skip the parts about Django and Confidential):

Blazing fast CI with GitHub Actions, Poetry, Black and Pytest

@buck54321
Copy link
Member

Poetry looks awesome. I'd appreciate if you would lead the effort, @teknico, though I might pump the brakes just a little bit. I won't be comfortable packaging until we're ready to tag a release, which will probably be a couple months. At a minimum, we'll want to 1) get the database upgrade in place, 2) finish revocations (#30 on the way), and 3) audit code, improve test coverage.

I don't consider it a priority to create a new repo right now, though I'm open to it. For now, I'll just propose the following reorganization, which will separate the packages.

├── LICENSE.txt
├── README.md
├── wallet
│   ├── ui
│   │   └── ...
│   ├── pyproject.toml
│   ├── requirements.txt
│   ├── app.py
│   └── config.py
└── dcrtools
    ├── pyproject.toml
    ├── requirements.txt
    ├── crypto
    │   └── ...
    ├── pydecred
    │   └── ...
    ├── util
    │   └── ...
    └─ ...

I like developing the wallet and toolset together because it encourages a clean but full-featured toolset API and more test coverage. That said, there's no reason this can't still work with separate repos. I'm not empowered to create new repositories for /decred, and my take so far is that Decred admins are pretty conservative about opening new repos. @dhill could maybe comment more.

I'll be opening a large DB upgrade PR within a few days. A repo reorg should follow that to prevent ridiculous rebase conflicts.

@teknico
Copy link
Collaborator Author

teknico commented Dec 24, 2019

It all seems reasonable. Tagging a release needs to happen before we can upload a package to PyPI, and I'd encourage to do that sooner rather than later. It can be a 0.Y.Z version, without API compatibility guarantees for now, but I understand the desire to get large in-progress features in from the start.

Splitting the GUI app into a separate repo is not mandatory, the layout you describe should work too. (We won't need requirements.txt files anymore though, that info will live inside pyproject.toml).

Agreed on waiting for the DB upgrade PR, we're in no hurry. And I have a testing proposal coming up anyway.

@rodrigondec
Copy link

I agree with having the first tagged release before doing this major modification.

Just want to say that what I'm interested is the python tool set which can be used to any other python developer (as myself) to interact with the Decred Blockchain (and not the tinywallet). So from my point of view (as a annoying customer wanting a new product) I'm waiting for some feature I don't need before the ones I want. And this is the primary reason for having these changes done.

I appreciate @teknico willingness and initiative of taking this task. Me and @fguisso will be here to support and help you as well (we are highly interested on this feature).

It will be easier with the new proposed organization by @buck54321 of this repo. And After this or in the meantime there's the possibility of using the new repo created (if there's any) as a git submodule (@fguisso called my attention to this sugestion). I don't know the feasibility of this but is something that could help.

@buck54321
Copy link
Member

I'd like to move forward on the repo restructure right after #47 is in. Now would be a good time to suggest any module renames or other reorganization steps not mentioned above.

After the restructure, we'll hit testing hard and then tag an alpha release and prepare a package.

@teknico
Copy link
Collaborator Author

teknico commented Jan 15, 2020

Resuming work on this, now that #42 and #47 have landed.

@teknico
Copy link
Collaborator Author

teknico commented Jan 15, 2020

The library and the wallet packages need the respective code and tests inside inner directories, so the layout has to be as follows (using dcrwallet because wallet is too generic a name for a Python package):

├── LICENSE
├── README.md
├── dcrwallet
│   ├── pyproject.toml
│   ├── app.py
│   ├── dcrwallet
│   │   ├── ui
│   │   │   └── ...
│   │   └── config.py
│   └── tests
│       └── ...
└── dcrtools
    ├── pyproject.toml
    ├── dcrtools
    │   ├── crypto
    │   │   └── ...
    │   ├── pydecred
    │   │   └── ...
    │   ├── util
    │   │   └── ...
    │   └── wallet
    │       └── ...
    └── tests
        └── ...

@buck54321
Copy link
Member

Do we need the extra level of nesting? Seems like we can

├── LICENSE
├── README.md
├── dcrwallet
│   ├── pyproject.toml
│   ├── app.py
│   ├── ui
│   └── config.py
│   └── tests
│       └── ...
└── dcrtools
    ├── pyproject.toml
    ├── crypto
    │   └── ...
    ├── pydecred
    │   └── ...
    ├── util
    │   └── ...
    ├── wallet
    │       └── ...
    └── tests
        └── ...

and then just specify what to include in the packages section of the pyproject.toml

@teknico
Copy link
Collaborator Author

teknico commented Jan 15, 2020

Do we need the extra level of nesting?

Maybe not, I'll try that.

@rodrigondec
Copy link

rodrigondec commented Jan 15, 2020

Yes, we need this additional dcrtools nesting.

If we follow this:

├── LICENSE
├── README.md
├── dcrwallet
│   ├── pyproject.toml
│   ├── app.py
│   ├── ui
│   └── config.py
│   └── tests
│       └── ...
└── dcrtools
    ├── pyproject.toml
    ├── crypto
    │   └── ...
    ├── pydecred
    │   └── ...
    ├── util
    │   └── ...
    ├── wallet
    │       └── ...
    └── tests
        └── ...

and then just specify what to include in the packages section of the pyproject.toml

These folders will be on the root of python env. So users will import like this:

from crypto import bla
from pydecred import foo
from wallet import bar

Instead of

from dcrtools.crypto import bla
from dcrtools.pydecred import foo
from dcrtools.wallet import bar

@buck54321
Copy link
Member

buck54321 commented Jan 16, 2020

I see. Let's roll with the default structure then.

re: names

dcrwallet is not great. I think people would assume it's a Python version of that project. The GUI app is actually called "tinydecred", so my first instinct is to name the package tinydecred, but I'm not married to the name. The important part for me is that we maintain a minimalist (tiny) UI, and the name was meant to reflect that principle.

If anyone wants to rename the wallet app, now is the time for suggestions. Keep in mind that multi-asset is approved in the current budget.

dcrtools name also hasn't received any scrutiny. Taking suggestions on that too.

@JoeGruffins
Copy link
Member

so, tinywallet?

@buck54321
Copy link
Member

I like that. I also suggested off-github to do pydecred instead of dcrtools, and rename the current pydecred module to dcr.

So right now, we're working with pydecred and tinywallet for package names. Still taking suggestions, but I'll start working on getting the pydecred name on PyPI.

@teknico
Copy link
Collaborator Author

teknico commented Jan 16, 2020

Well, a Python package name beginning with "py" is a bit redundant. We could call it just decred, or even tinydecred. The latter has the advantage of being available on PyPI.

@buck54321
Copy link
Member

buck54321 commented Jan 16, 2020

Getting decred should be easy. It's already orphaned because the owner deleted their account. The account becomes inactive in 10 days. There's also no actual code there. It just a setup.py.

As far as pydecred, it is redundant but it's also so common as to be expected. I bet if I asked a group of strangers what they would name the python package for Decred, most would instinctively answer "pydecred". It's also a little more relevant here, since Python is not Decred's "native" language. TD is broadly the "Python version" of Decred's Go tools, hence the py.

I do like just decred though. Hmmm.

@teknico
Copy link
Collaborator Author

teknico commented Jan 16, 2020

Initial changes in #59. @buck54321, I need you to choose between decred and pydecred. Toss a coin if you have to. 🙂

@buck54321
Copy link
Member

Use decred for now, but it's still up for discussion.

@rodrigondec
Copy link

rodrigondec commented Jan 16, 2020

pydecred is redundant, but it is a Decred implementation on python as @buck54321 mentioned.

There are others libs which do this and I like it

@teknico
Copy link
Collaborator Author

teknico commented Jan 20, 2020

As discussed above we're going to have two packages in this repo, the decred library and the tinywallet app.

While specifying the first one as a dependency for the second using a local relative path, I'm hitting a Poetry bug. I'm monitoring that issue.

We'll be able to stop using a local path and point instead to the Package Index once we have our packages up there. In the meantime the temporary workaround is to use an absolute path instead of a relative one: that's only a workaround because everyone's local absolute path is different.

@buck54321
Copy link
Member

Just a Poetry bug, right?

If so, don't let this stop the actual repo reorganization, even if we need to change the scope of #59 and follow up with packaging fixes.

Just to be sure, I use symlinks in a sys.path directory to point to my TD repo. I hope to still to do that after reorg, there'll just be two of them now.

@buck54321
Copy link
Member

Issue opened for PyPI decred name transfer, btw.

pypi/support#163

@teknico
Copy link
Collaborator Author

teknico commented Jan 20, 2020

Just a Poetry bug, right?

If so, don't let this stop the actual repo reorganization, even if we need to change the scope of #59 and follow up with packaging fixes.

Yes, I'm going ahead using the mentioned workaround for now.

Just to be sure, I use symlinks in a sys.path directory to point to my TD repo. I hope to still to do that after reorg, there'll just be two of them now.

The workflow will be different after reorg, you'll use the poetry command for everything. 🙂 It handles the virtualenv automatically, it's quite nice.

Issue opened for PyPI decred name transfer, btw.

Thanks for that, let's see what they say.

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

Successfully merging a pull request may close this issue.

4 participants