CZ-NIC / setuptools-grpc

Setuptools module for building protobuf and grpc service python modules.
GNU General Public License v3.0
5 stars 1 forks source link

Inject build subcommand #7

Closed stinovlas closed 8 months ago

stinovlas commented 1 year ago

This PR implements automatic injection of build_grpc command to setuptools. It's added before all other build subcommands, because those subcommands may need modules produced by build_grpc.

There is no way to disable this (other than not installing setuptools-grpc package). This should be fine, because build tool creates isolated environments where setuptools-grpc should not be installed unless intentionally requested. If anyone has a problem with this, we can add env variable to supress this behaviour (but we don't see much need for it at the moment, I'm just mentioning it as possible escape hatch).

This will make using setuptools-grpc easier, because we'll no longer have to override build command in setup.py (and it should be possible to omit setup.py all together.

tapetersen commented 8 months ago

When encountering the comments on not being able to do the configuring of subcommands automatically I thought of exactly this way and was prepared to write a similar pull-request. Is there anything that blocks forward progress of this that could help get it accepted (or explicitly rejected if not desired).

stinovlas commented 8 months ago

When encountering the comments on not being able to do the configuring of subcommands automatically I thought of exactly this way and was prepared to write a similar pull-request. Is there anything that blocks forward progress of this that could help get it accepted (or explicitly rejected if not desired).

Hi! I was playing with this, but I'm not sure whether we should proceed. The problem is that this behavior is not configurable. As long as setuptools-grpc is installed in your environment, it will inject itself to the build step. This would definitely be a breaking change demanding new major version release (and I was wondering whether we should proceed straight to 1.0.0, because it's already pretty stable otherwise).

Other than that, the PR's mostly ready (although I noticed some typos that I'd like to correct first).

Do you think it's worth it even though it's not configurable? I could also add some env variable that would be able to block the behaviour, such as NO_BUILD_GPRC. That would allow at least some kind of configuration, even when the sensible default is a breaking change.

I'd also like to hear @ziima's thoughts on this :slightly_smiling_face:.

tapetersen commented 8 months ago

Spontaneously without checking the config options and semantic clearer my instinct would be that it always indeed injects the command if installed but doesn't actually compile anything if no paths to search for files are configured. Will take a closer look later if that is reasonable here or explicit configuration of that behavior would be required.

I would lean on not providing an override for not injecting the command as you have to explicitly add this to the build-dependencies to have it enabled and I see very few instances where you would want that but not have it run (or at least wait until such a case is requested by someone).

stinovlas commented 8 months ago

Spontaneously without checking the config options and semantic clearer my instinct would be that it always indeed injects the command if installed but doesn't actually compile anything if no paths to search for files are configured. Will take a closer look later if that is reasonable here or explicit configuration of that behavior would be required.

That is correct, AFAIK.

I would lean on not providing an override for not injecting the command as you have to explicitly add this to the build-dependencies to have it enabled and I see very few instances where you would want that but not have it run (or at least wait until such a case is requested by someone).

I guess you're right. We can always add that later.

I had to clean up tests a bit, but everything seems to work now. When it passes the code review, I'll prepare a new version.

stinovlas commented 8 months ago

This has been released in version 1.0.0b1. I'd like to give it a week for testing before releasing stable version 1.0.0.