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.

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

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-12-18T20:57:19Z

Well I tried --output_user_root=\\?\C:\\tmp but it doesn't seem to work? Maybe there needs to be some sort of quoting?

danlapid · 2024-12-19T18:28:41Z

Well I tried --output_user_root=\\?\C:\\tmp but it doesn't seem to work? Maybe there needs to be some sort of quoting?

I think it should be "\\\\?\\C:\\tmp"

hoodmane · 2024-12-24T14:00:59Z

Using \\\?\\C:\\tmp seems to work but it just pushes the error back a bit. Now there are a ton of errors like:

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

hoodmane · 2024-12-24T14:33:16Z

All the too long paths have __tests__ in them, maybe we can filter those out while cloning...

danlapid · 2024-12-24T16:24:27Z

src/rust/cxx-integration/cxx-bridge.h

@@ -49,6 +49,14 @@ inline kj::ArrayPtr<const char> fromRust(const ::rust::Str& str) {
  return kj::ArrayPtr<const char>(str.data(), str.size());
 }

+inline kj::Array<kj::String> fromRust(::rust::Vec<::rust::String> vec) {


This copies 😢

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

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 3 times, most recently from 6c100fc to 225d6dc Compare November 13, 2024 20:59

hoodmane force-pushed the hoodmane/ruff-parser branch from 225d6dc to ee358fb Compare December 18, 2024 11:27

hoodmane added 2 commits December 18, 2024 15:54

Use ruff-python-parser to compute Python imports

28d804c

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.

Apply some of mikea's review comments

cf7021d

hoodmane force-pushed the hoodmane/ruff-parser branch from ee358fb to cf7021d Compare December 18, 2024 14:54

hoodmane added 3 commits December 18, 2024 20:26

Update dependencies

3578d10

Add missing crate

55eab71

Try UNC file name on windows

abfad1c

hoodmane requested a review from a team as a code owner December 18, 2024 20:28

Try more back slashes

b161e2c

hoodmane added 2 commits December 24, 2024 16:51

Try enabling core.longpaths

883b8e6

Add //deps/rust:runtime

8f6275d

danlapid reviewed Dec 24, 2024

View reviewed changes

danlapid approved these changes Dec 24, 2024

View reviewed changes

Fix format

ef39bb1

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

hoodmane commented Dec 18, 2024

danlapid commented Dec 19, 2024 •

edited

Loading

hoodmane commented Dec 24, 2024

hoodmane commented Dec 24, 2024

danlapid Dec 24, 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 Dec 18, 2024

danlapid commented Dec 19, 2024 • edited Loading

hoodmane commented Dec 24, 2024

hoodmane commented Dec 24, 2024

Choose a reason for hiding this comment

hoodmane commented Nov 13, 2024 •

edited

Loading

anonrig commented Nov 13, 2024 •

edited

Loading

danlapid commented Dec 19, 2024 •

edited

Loading