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

Dependencies: refactor with rules_python update #14329

Closed
wants to merge 16 commits into from
Closed

Dependencies: refactor with rules_python update #14329

wants to merge 16 commits into from

Conversation

moderation
Copy link
Contributor

Commit Message: Dependencies: refactor to remove bazel/repositories_extra.bzl with rules_python update
Additional Description:

  • rules_python 0.1.0 (release notes). Move pip imports from bazel/repositories_extra.bzl and consolidate into bazel/dependency_imports.bzl
  • change six from pythonhosted to Github location so we can verify release date
  • change layout of bazel/repository_locations.bzl so that release_date follows version and sha256 to aid with dependency maintainance
  • updated tools/dependency/release_dates.py to check for release metadata vs. current approach of checking for latest commit in a release which may not equal release date. Makes additional calls to the Github API and not super efficient but works. /cc @htuch for Python review.

Risk Level: Low
Testing: bazel --nohome_rc test //test/..., bazel --nohome_rc test @envoy_api_canonical//test/... @envoy_api_canonical//tools/..., bazel --nohome_rc build @envoy_api_canonical//envoy/..., tools/dependency/release_dates.py bazel/repository_locations.bzl
Docs Changes: None required
Release Notes: None required

Signed-off-by: Michael Payne [email protected]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Dec 8, 2020
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14329 was opened by moderation.

see: more, trace.

@htuch htuch self-assigned this Dec 8, 2020
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks! Overall approach makes sense, can you cleanup the Python (re: debug remnants and other nits) and I'll take another pass?
/wait

tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
bazel/repository_locations.bzl Outdated Show resolved Hide resolved
bazel/repository_locations.bzl Show resolved Hide resolved
bazel/repository_locations.bzl Outdated Show resolved Hide resolved
bazel/repository_locations.bzl Show resolved Hide resolved
Signed-off-by: Michael Payne <[email protected]>
@moderation moderation requested a review from htuch December 9, 2020 07:48
@moderation
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14329 (comment) was created by @moderation.

see: more, trace.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks, just a few logic questions.

tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
tools/dependency/release_dates.py Outdated Show resolved Hide resolved
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo wording nit.
/wait

tools/dependency/release_dates.py Outdated Show resolved Hide resolved
for tag in tags.reversed:
if tag.name == github_release.version:
current_metadata_tag_commit_date = tag.commit.commit.committer.date
# Exclude pre-releases like release candidates (eg. apache/kafka)
Copy link
Member

Choose a reason for hiding this comment

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

Something tells me that folks will have ways of creating tag names that are going to break us, but I'm happy to land what you have and iterate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a bunch of different approaches here with leading v on versions, straight numbers, some with underscores, some with dashes, some with both underscores and dashes and some with alpha numeric strings etc. I think the version import in packaging will help with this and so far the scripts runs OK against our repositories. But definitely a potential source of fragility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bigger issue here is a Windows CI failure with the new rules_python 0.1.0 approach - https://github.com/bazelbuild/rules_python/releases/tag/0.1.0. Fails consistently trying to build the Thrift python bits - https://dev.azure.com/cncf/envoy/_build/results?buildId=60601&view=logs&j=b840a642-5ff3-5357-2e4b-e06e40b0cffd&t=67965174-5100-5631-9dc0-68b9f0aacb53. Any ideas @envoyproxy/windows-dev ?

Copy link
Member

Choose a reason for hiding this comment

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

Honest answer is that I dont know why/how this error is coming from. There are multiple mentions across the internet and it seems that this helps other folks.

https://stackoverflow.com/questions/59115861/python-3-6-tensorflow-install-on-windows-failed-with-typeerror-stat-path-shou

Copy link
Member

Choose a reason for hiding this comment

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

I think rules_python might have to update its dependency on setuptools, it is pretty old and looks like the version in use has some incorrect logic around finding the vcruntime*.dll

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@sunjayBhatia sunjayBhatia Dec 21, 2020

Choose a reason for hiding this comment

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

Specifically in https://github.com/pypa/setuptools/blob/v42.0.2/setuptools/msvc.py#L1540-L1572

I think https://github.com/pypa/setuptools/blob/v42.0.2/setuptools/msvc.py#L1564-L1566 might be wrong

Looking at a build container these are all the paths I see vcruntime*.dll installed:

/c/build/tmp/external/remotejdk11_win/bin/vcruntime140.dll
/c/build/tmp/_bazel_ContainerAdministrator/install/7b45c91da117b160cc3b12126324930f/embedded_tools/jdk/bin/vcruntime140.dll
/c/Program Files/LLVM/bin/vcruntime140.dll
/c/Program Files/LLVM/bin/vcruntime140_1.dll
/c/Program Files/Python38/vcruntime140.dll
/c/Program Files/Python38/vcruntime140_1.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/debug_nonredist/x64/Microsoft.VC142.DebugCRT/vcruntime140d.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/debug_nonredist/x64/Microsoft.VC142.DebugCRT/vcruntime140_1d.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/onecore/debug_nonredist/x64/Microsoft.VC142.DebugCRT/vcruntime140d.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/onecore/debug_nonredist/x64/Microsoft.VC142.DebugCRT/vcruntime140_1d.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/onecore/x64/Microsoft.VC142.CRT/vcruntime140.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/onecore/x64/Microsoft.VC142.CRT/vcruntime140_1.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/x64/Microsoft.VC142.CRT/vcruntime140.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/x64/Microsoft.VC142.CRT/vcruntime140_1.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.27.29110/bin/Hostx64/x64/vcruntime140.dll
/c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Tools/MSVC/14.27.29110/bin/Hostx64/x64/vcruntime140_1.dll
/c/Windows/System32/vcruntime140.dll
/c/Windows/System32/vcruntime140d.dll
/c/Windows/System32/vcruntime140_1.dll
/c/Windows/System32/vcruntime140_1d.dll
/c/Windows/SysWOW64/vcruntime140.dll
/c/Windows/SysWOW64/vcruntime140d.dll

I think setuptools is looking for /c/Program Files (x86)/Microsoft Visual Studio/2019/BuildTools/VC/Redist/MSVC/14.27.29016/x64/Microsoft.VC142.CRT/vcruntime*.dll but the problematic part is the Microsoft.VC142.CRT rather than Microsoft.VC140.CRT in the path, which is incorrect for the latest versions of MSVC/VCRedist (or inconsistency from Microsoft, idk)

Copy link
Member

Choose a reason for hiding this comment

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

The logic around finding vcruntime*.dll has completely changed so might be worth an update: https://github.com/pypa/setuptools/blob/v51.1.0/setuptools/msvc.py#L217, looks like it handles that path better

Copy link
Member

Choose a reason for hiding this comment

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

This diff seems to have fixed things:

diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
index 92c2b0e84..f4ed59589 100644
--- a/bazel/repositories.bzl
+++ b/bazel/repositories.bzl
@@ -562,7 +562,11 @@ def _com_google_absl():
     )
 
 def _com_google_protobuf():
-    external_http_archive("rules_python")
+    external_http_archive(
+        name = "rules_python",
+        patches = ["@envoy//bazel:rules_python.patch"],
+        patch_args = ["-p1"],
+    )
     external_http_archive(
         "com_google_protobuf",
         patches = ["@envoy//bazel:protobuf.patch"],
diff --git a/bazel/rules_python.patch b/bazel/rules_python.patch
new file mode 100644
index 000000000..81929031d
--- /dev/null
+++ b/bazel/rules_python.patch
@@ -0,0 +1,15 @@
+diff --git a/python/pip_install/repositories.bzl b/python/pip_install/repositories.bzl
+index df63674..80824e4 100644
+--- a/python/pip_install/repositories.bzl
++++ b/python/pip_install/repositories.bzl
+@@ -16,8 +16,8 @@ _RULE_DEPS = [
+     ),
+     (
+         "pypi__setuptools",
+-        "https://files.pythonhosted.org/packages/54/28/c45d8b54c1339f9644b87663945e54a8503cfef59cf0f65b3ff5dd17cf64/setuptools-42.0.2-py2.py3-none-any.whl",
+-        "c8abd0f3574bc23afd2f6fd2c415ba7d9e097c8a99b845473b0d957ba1e2dac6",
++        "https://files.pythonhosted.org/packages/3d/f2/1489d3b6c72d68bf79cd0fba6b6c7497df4ebf7d40970e2d7eceb8d0ea9c/setuptools-51.0.0-py3-none-any.whl",
++        "8c177936215945c9a37ef809ada0fab365191952f7a123618432bbfac353c529",
+     ),
+     (
+         "pypi__wheel",

@htuch
Copy link
Member

htuch commented Dec 12, 2020

LGTM @moderation can you back out the problematic rules_python upgrade and do a dedicated PR for that? Seems like something we can hand-off to the Windows folk.

@davinci26
Copy link
Member

Taking a closer look now, trying to repro at my devbox @moderation

@mattklein123 mattklein123 assigned lizan and unassigned htuch Dec 18, 2020
@@ -211,6 +211,7 @@ build:remote --auth_enabled=true
build:remote --remote_download_toplevel

# Windows bazel does not allow sandboxed as a spawn strategy
build:remote-windows --action_env=WRAPT_INSTALL_EXTENSIONS=false
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this and the other change to this file

Base automatically changed from master to main January 15, 2021 23:02
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 22, 2021
@github-actions
Copy link

github-actions bot commented Mar 2, 2021

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants