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

BUG: fix broken packaging #327

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

BUG: fix broken packaging #327

wants to merge 13 commits into from

Conversation

Sann5
Copy link

@Sann5 Sann5 commented Jan 2, 2024

Closes #326
See issue above for detailed explanation.

@FriedrichFroebel
Copy link

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.

@Sann5
Copy link
Author

Sann5 commented Jan 3, 2024

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
Copy link
Author

Sann5 commented Jan 9, 2024

@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 )

@FriedrichFroebel
Copy link

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.

@Sann5
Copy link
Author

Sann5 commented Jan 9, 2024

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
Copy link
Author

Sann5 commented Jan 10, 2024

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
...

@FriedrichFroebel
Copy link

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.

@Sann5
Copy link
Author

Sann5 commented Jan 17, 2024

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

@harelba
Copy link
Owner

harelba commented Jan 17, 2024

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.

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 this pull request may close these issues.

BUG: Packaging in broken
3 participants