Skip to content

Trust google-auth library will handle their dependency properly#23200

Merged
lidizheng merged 2 commits intogrpc:masterfrom
lidizheng:fix-rsa-2
Jun 16, 2020
Merged

Trust google-auth library will handle their dependency properly#23200
lidizheng merged 2 commits intogrpc:masterfrom
lidizheng:fix-rsa-2

Conversation

@lidizheng
Copy link
Copy Markdown
Contributor

@lidizheng lidizheng commented Jun 12, 2020 •
edited
Loading

Earlier today, in #23197, we pinned rsa==4.3. However, that might not be a good long term solution. As stated, we don't directly depend on library rsa, but through google-auth. After all, google-auth has a better position to prevent further breakage.

So, this PR removes rsa from our dependency list, and allows our CI to pick up newer google-auth.

@lidizheng lidizheng added lang/Python release notes: no Indicates if PR should not be in release notes kokoro:force-run labels Jun 12, 2020
@lidizheng lidizheng mentioned this pull request Jun 12, 2020
Merged
gnossen
gnossen reviewed Jun 12, 2020
View reviewed changes
requirements.bazel.txt
idna==2.7
googleapis-common-protos==1.5.5
# rsa 4.3 is the last version support Python 2
rsa==4.3
Copy link
Copy Markdown
Contributor

@gnossen gnossen Jun 12, 2020

Choose a reason for hiding this comment

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

I'm pretty sure this will break. IIRC, rules_python requires you to identify versions for your entire dependency tree.

Copy link
Copy Markdown
Contributor Author

@lidizheng lidizheng Jun 12, 2020

Choose a reason for hiding this comment

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

I thought so, but rsa is a new dependency I manually added in #23197. If it wasn't breaking in the past, reverting should not break other stuff.

However, Python tests are still failing. I'm not sure where the rsa==4.0 slipped into our CI.

Copy link
Copy Markdown
Contributor

@gnossen gnossen Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Huh. Maybe google-auth only has a soft dependency on rsa?

Edit: That appears to be precisely what's happening.

Copy link
Copy Markdown
Contributor Author

@lidizheng lidizheng Jun 12, 2020

Choose a reason for hiding this comment

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

Nice finding!

@lidizheng
Copy link
Copy Markdown
Contributor Author

lidizheng commented Jun 16, 2020

@gnossen PTALA. The failed tests are irrelevant to this PR.

Previously, the installation of rsa resulted in a file creation contention. Multiple processes try to install rsa at the same time. It might due to some lazy installation feature of Python, but I couldn't find the root cause.

gnossen
gnossen approved these changes Jun 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this issue!

@lidizheng
Copy link
Copy Markdown
Contributor Author

lidizheng commented Jun 16, 2020

Known failures: #22019

@lidizheng lidizheng merged commit 09fa0ef into grpc:master Jun 16, 2020
@lidizheng lidizheng mentioned this pull request Jun 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnossen gnossen gnossen approved these changes

Assignees

@gnossen gnossen

Labels

lang/Python release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lidizheng @gnossen @grpc-kokoro