q icon indicating copy to clipboard operation
q copied to clipboard

BUG: fix broken packaging

Open Sann5 opened this issue 1 year ago • 9 comments

Closes #326 See issue above for detailed explanation.

Sann5 avatar Jan 02 '24 20:01 Sann5

Even with this patch, this does not look correct as this will install a package named bin when one wants to install the package q. The usual approach for packaging single-file distributions usually is to have the corresponding file inside the top-level directory (which will install the file into the site-packages without a wrapping directory) or to use q/__init__.py (and maybe q/__main__.py for CLI-related stuff) instead.

FriedrichFroebel avatar Jan 03 '24 07:01 FriedrichFroebel

I agree. I mentioned this in the issue linked in the description (additional considerations section). But since it involves a more serious refactoring I was hoping to get the opinion from one of the maintainers before moving forward.

Sann5 avatar Jan 03 '24 10:01 Sann5

@FriedrichFroebel what would you like me to name the package? I also move the tests into the package so that they can be run by a CI (need this to make q available in conda. See this issue: #322 )

Sann5 avatar Jan 09 '24 17:01 Sann5

what would you like me to name the package?

Usually q.

I also move the tests into the package so that they can be run by a CI

This should not be required if you do proper packaging of source and binary distributions. The tests directory can stay at the root level of the repository and should be packaged inside the sdist by default, while binary distributions will only contain the actual q code without the tests.

FriedrichFroebel avatar Jan 09 '24 17:01 FriedrichFroebel

Putting the tests inside the src directory makes it easier to run the tests in the CI for conda forge. I can try to leave them as is but if it's not a big deal to you I would suggest leaving them inside. I think tests are nice when making a tool like this available (for example I discovered that q can break with Python >= 3.11). What's more, there is no binary for Linux which is my main motivation for making q available in conda.

Sann5 avatar Jan 09 '24 17:01 Sann5

This is the layout I propose based on what I have seen on best practices, naming conventions, and ease of testing @FriedrichFroebel.

...
├── setup.py
├── src
│   ├── q
│   │   ├── __init__.py
│   │   ├── q.bat
│   │   ├── q.py
│   │   └── tests
│   │       ├── BENCHMARK.md
│   │       ├── __init__.py
│   │       ├── benchmark-results
│   │       │   └── ...
│   │       ├── data
│   │       │   ├── EXAMPLES.markdown
│   │       │   ├── exampledatafile
│   │       │   └── group-emails-example
│   │       └── test_suite.py
...

Sann5 avatar Jan 10 '24 10:01 Sann5

As already said and mentioned inside the pytest docs as well: This depends on the preferred distribution model and some other factors. I tend to lean towards a strict separation of binary distributions (wheels) with the actual application code only and a separate source distribution (sdist) with all the source files (including tests) to use for building own wheels, OS-specific packages etc.

Shipping the tests inside a dedicated package makes the actual installation from wheels smaller as it does not have to ship the testing stuff - this is the preferred way for libraries which are used in larger projects etc. where we can assume that the package itself is well-tested in an isolated manner.

Shipping the tests inside the actual package tends to bloat the wheels and only makes sense if you want to run the tests from within a larger application, which does not make much sense in most of the cases.

FriedrichFroebel avatar Jan 15 '24 19:01 FriedrichFroebel

@FriedrichFroebel I understand. I restored the original project structure. Let me know if there is anything else that doesn't check out.

Sann5 avatar Jan 17 '24 10:01 Sann5

I believe that i'm using an old ubuntu version for the github actions build/test workflow, and therefore some of the workflow jobs are infinitely queued without running.

I will try to push some changes to the github actions to see if i can make them run properly.

harelba avatar Jan 17 '24 23:01 harelba