✨ Add preliminary python support #348

JonahSussman · 2023-09-28T19:52:26Z

Big PR, hopefully I didn't break anything

Signed-off-by: JonahSussman <[email protected]>

Signed-off-by: Jonah Sussman <[email protected]>

Signed-off-by: JonahSussman <[email protected]>

pranavgaikwad · 2023-09-29T15:22:18Z

.github/workflows/demo-testing.yml

      - name: run demo image and ensure violations output unchanged
        run: |
          podman run -v $(pwd)/demo-output.yaml:/analyzer-lsp/output.yaml:Z localhost/testing:latest
-          git diff --exit-code demo-output.yaml
+          diff \
+            <(sort <(sed 's/incidents\.[0-9]\+/incidents\.x/g' <(yq -P 'sort_keys(..)' -o=props <(git show HEAD:demo-output.yaml)))) \


what sorcery is this?

Since the incidents in a violation can be returned in any order, git diff kept failing. i.e. git diff would compare

sample-rule: incidents: - A - B

sample-rule: incidents: - B - A

and fail. This is because diff only compares the textual differences between the files. It has no knowledge of the YAML structure. To fix this, I installed yq which allows us to examine the actual structure of the input YAML. My whole goal was to still use diff to compare the two files, but make the position of each incident in the array irrelevant.

Moving from right to left, I first get the different versions of demo-output.yaml. I then pipe that into yq, with the -o=props flag . -P 'sort_keys(..)' is actually unnecessary due to what I do later. If we piped the YAML files I showed into that, it would return:

sample-rule.incidents.0 = A sample-rule.incidents.1 = B

sample-rule.incidents.0 = B sample-rule.incidents.1 = A

I then pipe that output into sed, replacing every incidents.<number> with incidents.x to erase which index each element is in. Finally, I sort the result and diff the two streams. Both files would return

sample-rule.incidents.x = A sample-rule.incidents.x = B

Which is the same thing. No error, ta-da!

There might be a better way than cobbling together some shell commands for this. I wouldn't even be surprised if there was a way you could do all of this in yq! At least one of the advantages of having it installed is that we can investigate the actual structure of the YAML now in our tests.

@JonahSussman lovely, I think this is a much better version of what we are doing in Kantra now, would love to copy it there

I would consider it a great honor, copy away! I would remove the -P 'sort_keys(..)' part as I'm almost positive it's redundant.

Can we sort these in the engine then?

djzager

This looks good, a few comments but I don't believe any of them are blockers.

I am curious though, after looking into our fallback symbol searching, if we could accept some funcs for the handling of some of the differences between different language servers. That may enable us to make the generic provider work down to the lowest common denominator and have specific examples for golang/pythong/typescript/etc on how to extend it to handle peculiarities between the different implementations.

djzager · 2023-10-03T15:36:24Z

external-providers/generic-external-provider/main.go

+	client := generic.NewGenericProvider()
+
+	golangSC, err := client.Init(ctx, log, provider.InitConfig{
+		Location: "/home/jonah/Projects/analyzer-lsp/examples/golang",


This path doesn't look like one we'd want to commit.

IMO, Eiher we remove this, or we make this more generic for testing.

I would think that removing for this PR and then coming back and making it easy to test w/ the generic provider makes sense to me. OR we make this a feature of kantra, and work on it from that the layer?

IDK just don't know if we want to change this particular main file yet but I could be wrong

Whoops, will fix! Is leaving in the mainDebug() function a problem as well? I just found all the stuff I put in there extremely useful when I was having problems.

djzager · 2023-10-03T15:48:27Z

external-providers/generic-external-provider/pkg/generic/service_client.go

-func (p *genericServiceClient) GetAllSymbols(ctx context.Context, query string) []protocol.WorkspaceSymbol {
+// Returns all symbols for the given query.
+// NOTE: Only returns definitions when server does not supoprt workspace/symbol.
+// Is is intended behavior?


For the .NET work, what made the most sense while experimenting was to:

find occurrences of pattern

call "textDocument/definition" and see if the response URI includes the referenced namespace (ie. System.Web.Mvc)...if it does then we add it as an incident.

And, iirc @fabianvf had mentioned gopls handling references differently which would mean that this is yet another area where differences in implementation may bite us. Makes me wonder if we want to make it possible to pass a callback of sorts that allows you to specify a function for determining if a found Symbol using the fallback method should be included as an incident.

If I'm understanding right, you're saying the .NET work utilizes the fallback method? I added the note as if we use the fallback method, only the definitions will be returned, rather than every symbol like when using workspace/symbol. Honestly, I'm not quite seeing why we should do anything other than the fallback method?

I copy/pasted and then modified the fallback method for what I was doing with the .NET work. See here.

My thought was that it may be possible for us to create an exported method for our fallback that accepts a func for determining if a given symbol is the one the provider is looking for.

Looking more thoroughly at what I did, it may be best for us to have our fallback method as an exported function that simply returns a slice of positions...leaving it up to the caller on how to sort which of those positions are valid symbols or not.

djzager · 2023-10-03T15:51:34Z

lsp/protocol/extensions.go

@@ -0,0 +1,151 @@
+package protocol


I am hopeful that we can move to https://github.com/go-language-server/protocol but I think that's an important-long-term type issue. Do we agree? I can writeup the issue later.

Definitely something I want to investigate. Any way we can make it less brittle is a win in my book

shawn-hurley

More of slight clean ups and follow up issues that need to be created.

I do want to understand why we are exposing the ServiceClient. I thought that the interface should fully cover what anything else needed to do with it.

shawn-hurley · 2023-10-03T16:21:02Z

.github/workflows/demo-testing.yml

      - name: run demo image and ensure violations output unchanged
        run: |
          podman run -v $(pwd)/demo-output.yaml:/analyzer-lsp/output.yaml:Z localhost/testing:latest
-          git diff --exit-code demo-output.yaml
+          diff \
+            <(sort <(sed 's/incidents\.[0-9]\+/incidents\.x/g' <(yq -P 'sort_keys(..)' -o=props <(git show HEAD:demo-output.yaml)))) \


Can we sort these in the engine then?

shawn-hurley · 2023-10-03T16:22:39Z

Dockerfile

@@ -21,6 +21,9 @@ FROM jaegertracing/all-in-one:latest AS jaeger-builder
 # The unofficial base image w/ jdtls and gopls installed
 FROM quay.io/konveyor/jdtls-server-base

+RUN python3 -m ensurepip --upgrade


Note to selves, we need to eventually find a way to have these broken up and use some shared filesystem while running. Probably something for Kantra to figure out. Do we have an issue for this?

shawn-hurley · 2023-10-03T16:26:26Z

external-providers/generic-external-provider/main.go

+	client := generic.NewGenericProvider()
+
+	golangSC, err := client.Init(ctx, log, provider.InitConfig{
+		Location: "/home/jonah/Projects/analyzer-lsp/examples/golang",


IMO, Eiher we remove this, or we make this more generic for testing.

I would think that removing for this PR and then coming back and making it easy to test w/ the generic provider makes sense to me. OR we make this a feature of kantra, and work on it from that the layer?

IDK just don't know if we want to change this particular main file yet but I could be wrong

shawn-hurley · 2023-10-03T16:29:54Z

external-providers/generic-external-provider/pkg/generic/service_client.go

-	rpc        *jsonrpc2.Conn
-	cancelFunc context.CancelFunc
-	cmd        *exec.Cmd
+type GenericServiceClient struct {


Why exporting the service client?

Had it for debugging stuff, changing it back

shawn-hurley · 2023-10-03T16:31:15Z

external-providers/generic-external-provider/pkg/generic/tables.go

+
+// 2-tiered table for each language and their respective fixes we may need to
+// apply
+var SubstitutionTable = map[string]map[string]string{


Can you add to the documentation for the specific python one why we need to substitute what? I can see the diff but am confused about the why.

Will add to the python-provider.md doc!

shawn-hurley · 2023-10-03T16:32:50Z

provider/internal/grpc/README.md

@@ -1,5 +1,17 @@
-To update the Provider GRPC definition, update `library.proto` and run:
+To install protoc, go to https://github.com/protocolbuffers/protobuf/releases 


This will only work for Linux, I would recommend that we point to the install docs for protoc instead.

shawn-hurley · 2023-10-03T16:35:06Z

provider/provider.go

@@ -102,12 +102,25 @@ const (
 type InitConfig struct {
 	// This is the location of the code base that the
 	// Provider will be responisble for parsing
+	// Deprecated: rootUri, which is what this maps to in the LSP spec, is deprecated.


Can you create issues in the repo for this and link in the deprecated or a TODO?

JonahSussman · 2023-10-03T16:47:41Z

This looks good, a few comments but I don't believe any of them are blockers.

I am curious though, after looking into our fallback symbol searching, if we could accept some funcs for the handling of some of the differences between different language servers. That may enable us to make the generic provider work down to the lowest common denominator and have specific examples for golang/pythong/typescript/etc on how to extend it to handle peculiarities between the different implementations.

@djzager I really want to investigate a good way to do this. The vast majority of the functionality between different providers seems the same. Would love to have a discussion about what that looks like

…"Run" go-routine Signed-off-by: JonahSussman <[email protected]>

Signed-off-by: JonahSussman <[email protected]>

shawn-hurley

nit's but the core is good

shawn-hurley · 2023-10-09T15:41:00Z

external-providers/generic-external-provider/pkg/generic/provider.go

-		cancelFunc: cancelFunc,
-		cmd:        cmd,
-		config:     c,
+		Rpc:        rpc,


nit: If changing, then it should be RPC

shawn-hurley · 2023-10-09T15:41:20Z

external-providers/generic-external-provider/pkg/generic/provider.go

-		config:     c,
+		Rpc:        rpc,
+		CancelFunc: cancelFunc,
+		Cmd:        cmd,


CMD as well

shawn-hurley · 2023-10-09T15:43:47Z

jsonrpc2/jsonrpc2.go

@@ -127,6 +139,9 @@ func (c *Conn) Call(ctx context.Context, method string, params, result interface
 			h.Done(ctx, err)
 		}
 	}()
+
+	// fmt.Printf("Sending: %s\n", string(data))


lets remove this instead of commenting IMO

Signed-off-by: JonahSussman <[email protected]>

JonahSussman and others added 8 commits September 11, 2023 23:18

Updated protocol to more recent version

f6e0d36

Signed-off-by: JonahSussman <[email protected]>

Update types for robustness

6584079

Signed-off-by: JonahSussman <[email protected]>

Merge branch 'konveyor:main' into update-protocol

8448e49

Resolve merge conflicts

6982513

Signed-off-by: JonahSussman <[email protected]>

Merge branch 'main' into update-protocol

6681eeb

Signed-off-by: Jonah Sussman <[email protected]>

Added python support via generic-external-provider

f55d500

Signed-off-by: JonahSussman <[email protected]>

Merge branch 'main' into python-exploration

44af4d8

Signed-off-by: JonahSussman <[email protected]>

Fixed Python venv example and demo-testing.yml

d48a9d5

Signed-off-by: JonahSussman <[email protected]>

JonahSussman requested review from fabianvf and pranavgaikwad September 28, 2023 19:52

pranavgaikwad reviewed Sep 29, 2023

View reviewed changes

djzager reviewed Oct 3, 2023

View reviewed changes

shawn-hurley requested changes Oct 3, 2023

View reviewed changes

JonahSussman added 2 commits October 6, 2023 02:13

Fixed issue where nonconforming notification responses gummed up the …

eb032bc

…"Run" go-routine Signed-off-by: JonahSussman <[email protected]>

Fixed requested changes

024f778

Signed-off-by: JonahSussman <[email protected]>

JonahSussman requested a review from shawn-hurley October 6, 2023 20:16

shawn-hurley approved these changes Oct 9, 2023

View reviewed changes

Fixed nits and added more robust jsonrpc2 debugger

1ddb892

Signed-off-by: JonahSussman <[email protected]>

JonahSussman merged commit b6195b3 into konveyor:main Oct 10, 2023
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add preliminary python support #348

✨ Add preliminary python support #348

JonahSussman commented Sep 28, 2023

pranavgaikwad Sep 29, 2023

JonahSussman Sep 29, 2023 •

edited

Loading

pranavgaikwad Sep 29, 2023

JonahSussman Sep 29, 2023

shawn-hurley Oct 3, 2023

djzager left a comment

djzager Oct 3, 2023

shawn-hurley Oct 3, 2023

JonahSussman Oct 3, 2023

djzager Oct 3, 2023

JonahSussman Oct 3, 2023

djzager Oct 4, 2023

djzager Oct 4, 2023

djzager Oct 3, 2023

JonahSussman Oct 3, 2023

shawn-hurley left a comment

shawn-hurley Oct 3, 2023

shawn-hurley Oct 3, 2023

shawn-hurley Oct 3, 2023

shawn-hurley Oct 3, 2023

JonahSussman Oct 6, 2023

shawn-hurley Oct 3, 2023

JonahSussman Oct 6, 2023

shawn-hurley Oct 3, 2023

shawn-hurley Oct 3, 2023

JonahSussman Oct 6, 2023

JonahSussman commented Oct 3, 2023

shawn-hurley left a comment

shawn-hurley Oct 9, 2023

shawn-hurley Oct 9, 2023

shawn-hurley Oct 9, 2023

		@@ -1,5 +1,17 @@
		To update the Provider GRPC definition, update `library.proto` and run:
		To install protoc, go to https://github.com/protocolbuffers/protobuf/releases

✨ Add preliminary python support #348

✨ Add preliminary python support #348

Conversation

JonahSussman commented Sep 28, 2023

Choose a reason for hiding this comment

JonahSussman Sep 29, 2023 • edited Loading

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

djzager 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

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

shawn-hurley 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

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

JonahSussman commented Oct 3, 2023

shawn-hurley 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

JonahSussman Sep 29, 2023 •

edited

Loading