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.

Sign up for GitHub

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

Jump to bottom

Removing googleapis-common-protos from deps in non-core packages. #4098

Merged
merged 3 commits into from
Oct 4, 2017
Merged

Removing googleapis-common-protos from deps in non-core packages. #4098

merged 3 commits into from
Oct 4, 2017

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 2, 2017

Also

  • removing grpcio from non-core packages.
  • manually specifying the grpcio dep in core (rather than getting it from googleapis-common-protos[grpc])

(NOTE: This is a followup to #4096.)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2017
tseaver
tseaver reviewed Oct 2, 2017
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

This makes grpcio an unconditional dependency for all APIs, even storage which is otherwise REST-only. Is that what we want?

@dhermes
Copy link
Contributor Author

dhermes commented Oct 2, 2017

@tseaver It already was, this is the line being removed from core/setup.py:

'googleapis-common-protos[grpc] >= 1.5.3, < 2.0dev',

@tseaver
Copy link
Contributor

tseaver commented Oct 2, 2017

@dhermes Yeah, but literally just now in #4096, right?

@dhermes
Copy link
Contributor Author

dhermes commented Oct 2, 2017

Ahh good call. That was some extra fail copy-pasta by me in #4096.

@tseaver
Copy link
Contributor

tseaver commented Oct 2, 2017

ISTM that adding unconditional dependencies to an already-GA API should be worth some debate.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 2, 2017

@tseaver Yes I did it by mistake in #4096, will undo here.

@dhermes
Removing googleapis-common-protos from deps in non-core packages.
dde1aba 
Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)
@dhermes
Copy link
Contributor Author

dhermes commented Oct 3, 2017

@tseaver PTAL, I made grpc an extra in core.

@jonparrott Does this look like the right approach for packaging?

dhermes
dhermes commented Oct 3, 2017
View reviewed changes
videointelligence/setup.py
@@ -40,6 +40,7 @@
install_requires=(
'googleapis-common-protos >= 1.5.3, < 2.0dev',
'google-gax >= 0.15.14, < 0.16dev',
'grpcio >= 1.2.0, < 1.6dev',

This comment was marked as spam.

Sign in to view

tseaver
tseaver approved these changes Oct 4, 2017
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

The grpc extra (and depending on it from the grpc-only API packages) looks right to me.

@dhermes dhermes merged commit d3ef455 into googleapis:master Oct 4, 2017
@dhermes dhermes deleted the update-grpcio-deps branch October 4, 2017 19:45
@dhermes
Copy link
Contributor Author

dhermes commented Oct 4, 2017

@lukesneeringer Can you follow-up on my question (#4098 (comment)) when you get a chance?

@liyanhui1228 liyanhui1228 mentioned this pull request Oct 12, 2017
Merged
@lukesneeringer lukesneeringer mentioned this pull request Oct 13, 2017
Merged
@theacodes theacodes mentioned this pull request Oct 16, 2017
Closed
atulep pushed a commit that referenced this pull request Apr 3, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
53f18e6 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
atulep pushed a commit that referenced this pull request Apr 18, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
fa5c50b 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
parthea pushed a commit that referenced this pull request Jul 6, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
f67d2ff 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
parthea pushed a commit that referenced this pull request Sep 22, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
d864ee7 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
parthea pushed a commit that referenced this pull request Oct 21, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
cfb9a58 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
parthea pushed a commit that referenced this pull request Oct 22, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
ef13273 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
parthea pushed a commit that referenced this pull request Oct 22, 2023
@dhermes
Removing googleapis-common-protos from deps in non-core packages. (…
7f848b5 
…#4098)

* Removing `googleapis-common-protos` from deps in non-`core` packages.

Also

- removing `grpcio` from non-`core` packages.
- manually specifying the `grpcio` dep in core (rather than getting
  it from `googleapis-common-protos[grpc]`)

* Making `grpc` an extra for `core`.

* Adding `googleapis-common-protos` back to `videointelligence`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theacodes theacodes theacodes approved these changes

@tseaver tseaver tseaver approved these changes

Assignees

@lukesneeringer lukesneeringer

@tseaver tseaver

Labels
cla: yes This human has signed the Contributor License Agreement. packaging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhermes @tseaver @theacodes @lukesneeringer @googlebot