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

publish wheels #534

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

publish wheels #534

wants to merge 1 commit into from

Conversation

dimbleby
Copy link

@dimbleby dimbleby commented Nov 27, 2024

Direct invocation of setup.py is deprecated

It is desirable to publish wheels. Else everyone who installs your package has to build the wheel themselves: which is slower, and might conceivably go wrong.

Better for package maintainers to build their wheels once and for all rather than make everyone else do it, every time.


This change is Reviewable

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dimbleby)


Makefile line 36 at r1 (raw file):

.PHONY : docs
docs:
	python -m pip install -r docs/requirements.txt

Isn't this modifying local environment?!
I'm pretty sure that building documentation shouldn't silently install packages


Makefile line 40 at r1 (raw file):

dist: docs
	python -m pip install build

why not just python -m build?

@dimbleby
Copy link
Author

it is not clear to me how you prefer to set up your virtual environment, feel free to make tweaks as you like. What I am mostly interested in encouraging is: please build (and publish) wheels as well as sdists.

but the make docs does not succeed unless you have installed the docs-requirements, and the make dist does not succeed unless you install the build package - so I think that my arrangement is reasonable

@tomato42
Copy link
Member

What I am mostly interested in encouraging is: please build (and publish) wheels as well as sdists.

sure, can do 🙂

but the make docs does not succeed unless you have installed the docs-requirements, and the make dist does not succeed unless you install the build package - so I think that my arrangement is reasonable

well, I believe in the principle of least surprise: commands called "build" should not "install" packages

it's normal in other projects that ./configure fails because you don't have dependencies installed, handling them is done through documentation, not trying to do it manually (maybe they want to install modules through their package manager, maybe they forgot to activate their virtual environment... too much guesswork)

@dimbleby
Copy link
Author

dimbleby commented Nov 27, 2024

As I say, feel free to make rearrangements to suit your workflow.

There is more than one way to skin a cat, IMO using a makefile to install dependencies is absolutely fine, but I have no interest in persuading you to do something you do not want to do.

Please make whatever changes you like to this pull request.

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.

2 participants