Use ruff-python-parser to compute Python imports #3103

hoodmane · 2024-11-13T16:10:06Z

The goal is to use ruff to replace Dominik's ad-hoc parser. This just adds the
new code, I'll switch to using it and remove the old code in a followup.

hoodmane · 2024-11-13T16:11:58Z

@mikea when I try to run this with:

bazel test src/rust/python-parser:import_parsing

I get:

In file included from external/crates_vendor__cxx-1.0.129/src/cxx.cc:1:
external/crates_vendor__cxx-1.0.129/src/../include/cxx.h:2:10: fatal error: 'algorithm' file not found
    2 | #include <algorithm>
      |          ^~~~~~~~~~~
1 error generated.
ERROR: /home/rchatham/.cache/bazel/_bazel_rchatham/494cfed3ffd15c51a9fc504927f340c6/external/capnp-cpp/src/kj/BUILD.bazel:5:11: Compiling src/kj/encoding.c++ failed: (Exit 1): clang failed: error executing CppCompile command (from target @@capnp-cpp//src/kj:kj)

any ideas what I'm doing wrong?

hoodmane · 2024-11-13T16:14:04Z

I get the same error when I try to run bazel test src/rust/cxx-integration-test:cxx-rust-integration-test hmm.

mikea · 2024-11-13T16:23:34Z

CI has a different error. You need this:

$ git diff
diff --git a/src/rust/python-parser/import_parsing.c++ b/src/rust/python-parser/import_parsing.c++
index 83b1a5cd..8d89f58b 100644
--- a/src/rust/python-parser/import_parsing.c++
+++ b/src/rust/python-parser/import_parsing.c++
@@ -2,7 +2,7 @@
 // Licensed under the Apache 2.0 license found in the LICENSE file or at:
 //     https://opensource.org/licenses/Apache-2.0
 
-#include <rust/python-parser/lib.rs.h>
+#include <workerd/rust/python-parser/lib.rs.h>
 
 #include <kj/test.h>

Workerd rust includes are prefixed with "workerd/" to disambiguate with edgeworker rust folders.

mikea · 2024-11-13T16:25:22Z

ERROR: /home/rchatham/.cache/bazel/_bazel_rchatham/494cfed3ffd15c51a9fc504927f340c6/external/capnp-cpp/src/kj/BUILD.bazel:5:11: Compiling src/kj/encoding.c++ failed: (Exit 1): clang failed: error executing CppCompile command (from target @@capnp-cpp//src/kj:kj)

something is broken for you since it can't even build kj:kj target (which has nothing to do with rust). I assume your environment is broken? This happens to me with workerd everytime I touch anything on my system.

hoodmane · 2024-11-13T16:28:02Z

Thanks @mikea! I got it to work by running it from edgeworker, and get the same path error. After fixing that it seems to works correctly.

The goal is to use ruff to replace Dominik's ad-hoc parser. This just adds the new code, I'll switch to using it and remove the old code in a followup.

anonrig · 2024-11-13T17:13:37Z

src/rust/python-parser/lib.rs

+
+#[cxx::bridge(namespace = "edgeworker::rust::python_parser")]
+mod ffi {
+


I don't think we have formatting enabled for Rust files. Can you run just rustfmt to see if any formatting changes are being made? If yes, we need to fix this (in a separate PR)

we do and --config=lint should be checking it. Let me know if this is not the case.

I ran this and don't see any changes. Maybe it's already set up correctly?

mikea

Awesome to see rust usage!

Just a lot of nits, sorry.

mikea · 2024-11-13T17:17:59Z

deps/rust/cargo.bzl

@@ -21,6 +21,8 @@ PACKAGES = {
    "pico-args": crate.spec(version = "0"),
    "proc-macro2": crate.spec(version = "1"),
    "quote": crate.spec(version = "1"),
+    "ruff_python_ast": crate.spec(git = "https://github.com/astral-sh/ruff.git", tag = "0.7.0"),


any idea why the don't publish on crates.io ?

My guess is that they have a big monorepo and don't really care about supporting downstream users of the crates only of the CLI tools they are used to define.

mikea · 2024-11-13T17:18:28Z

deps/rust/cargo.bzl

@@ -21,6 +21,8 @@ PACKAGES = {
    "pico-args": crate.spec(version = "0"),
    "proc-macro2": crate.spec(version = "1"),
    "quote": crate.spec(version = "1"),
+    "ruff_python_ast": crate.spec(git = "https://github.com/astral-sh/ruff.git", tag = "0.7.0"),


this will need downstream changes to add these crates too.

deps/rust/cargo.bzl

mikea · 2024-11-13T17:20:34Z

src/rust/python-parser/lib.rs

+
+#[cxx::bridge(namespace = "edgeworker::rust::python_parser")]
+mod ffi {
+


we do and --config=lint should be checking it. Let me know if this is not the case.

mikea · 2024-11-13T17:21:30Z

src/rust/python-parser/lib.rs

+mod ffi {
+
+    extern "Rust" {
+        fn get_imports(sources: &Vec<String>) -> Vec<String>;


let's add some /// comments here. They will be visible from c++ code too.
Don't forget to document sorting behavior.

src/rust/python-parser/lib.rs

src/rust/python-parser/import_parsing.c++

mikea · 2024-11-13T17:34:18Z

ouch. Windows as usual. The name doesn't seem to be really long, is windows limit so low??? Can we raise it?

error: unable to create file crates/ruff_linter/src/rules/flake8_quotes/snapshots/ruff_linter__rules__flake8_quotes__tests__require_docstring_doubles_over_docstring_singles_mixed_quotes_module_singleline_var_1.py.snap: Filename too long
error: unable to create file crates/ruff_linter/src/rules/flake8_quotes/snapshots/ruff_linter__rules__flake8_quotes__tests__require_docstring_doubles_over_docstring_singles_mixed_quotes_module_singleline_var_2.py.snap: Filename too long
error: unable to create file crates/ruff_linter/src/rules/flake8_quotes/snapshots/ruff_linter__rules__flake8_quotes__tests__require_docstring_singles_over_docstring_doubles_mixed_quotes_module_singleline_var_1.py.snap: Filename too long
error: unable to create file crates/ruff_linter/src/rules/flake8_quotes/snapshots/ruff_linter__rules__flake8_quotes__tests__require_docstring_singles_over_docstring_doubles_mixed_quotes_module_singleline_var_2.py.snap: Filename too long

anonrig · 2024-11-13T17:59:57Z

@mikea we need to add a prefix for UNC paths to avoid such errors.

Using "\\? \" permits a maximum path length of approximately 32,000 characters composed of components up to 255 characters in length. To specify a UNC path, use the "\\? \UNC\" prefix.

mikea · 2024-11-13T18:12:44Z

@mikea we need to add a prefix for UNC paths to avoid such errors.

Using "\\? \" permits a maximum path length of approximately 32,000 characters composed of components up to 255 characters in length. To specify a UNC path, use the "\\? \UNC\" prefix.

we can try doing it for C:\tmp that we set for bazel root

hoodmane force-pushed the hoodmane/ruff-parser branch from 2a6a506 to 7ada9d9 Compare November 13, 2024 16:29

hoodmane marked this pull request as ready for review November 13, 2024 16:30

hoodmane requested review from a team as code owners November 13, 2024 16:30

hoodmane requested review from mikea and vickykont November 13, 2024 16:30

Use ruff-python-parser to compute Python imports

51afa67

The goal is to use ruff to replace Dominik's ad-hoc parser. This just adds the new code, I'll switch to using it and remove the old code in a followup.

hoodmane force-pushed the hoodmane/ruff-parser branch from 7ada9d9 to 51afa67 Compare November 13, 2024 16:33

anonrig reviewed Nov 13, 2024

View reviewed changes

mikea reviewed Nov 13, 2024

View reviewed changes

hoodmane force-pushed the hoodmane/ruff-parser branch 2 times, most recently from 53bcabe to 6c100fc Compare November 13, 2024 20:57

Apply some of mikea's review comments

225d6dc

hoodmane force-pushed the hoodmane/ruff-parser branch from 6c100fc to 225d6dc Compare November 13, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruff-python-parser to compute Python imports #3103

Use ruff-python-parser to compute Python imports #3103

hoodmane commented Nov 13, 2024 •

edited

Loading

hoodmane commented Nov 13, 2024

hoodmane commented Nov 13, 2024

mikea commented Nov 13, 2024

mikea commented Nov 13, 2024

hoodmane commented Nov 13, 2024

anonrig Nov 13, 2024

mikea Nov 13, 2024

hoodmane Nov 13, 2024

mikea left a comment

mikea Nov 13, 2024

hoodmane Nov 13, 2024

mikea Nov 13, 2024

mikea Nov 13, 2024

mikea Nov 13, 2024

mikea commented Nov 13, 2024

anonrig commented Nov 13, 2024 •

edited

Loading

mikea commented Nov 13, 2024


		#[cxx::bridge(namespace = "edgeworker::rust::python_parser")]
		mod ffi {

Use ruff-python-parser to compute Python imports #3103

Are you sure you want to change the base?

Use ruff-python-parser to compute Python imports #3103

Conversation

hoodmane commented Nov 13, 2024 • edited Loading

hoodmane commented Nov 13, 2024

hoodmane commented Nov 13, 2024

mikea commented Nov 13, 2024

mikea commented Nov 13, 2024

hoodmane commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikea commented Nov 13, 2024

anonrig commented Nov 13, 2024 • edited Loading

mikea commented Nov 13, 2024

hoodmane commented Nov 13, 2024 •

edited

Loading

anonrig commented Nov 13, 2024 •

edited

Loading