-
Notifications
You must be signed in to change notification settings - Fork 69
Support pre-compressed uploads in code-intel upload, deprecate LSIF #1146
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
Conversation
e7234d0
to
f9592c2
Compare
You mean the one in this repo itself? Yeah, sure, that seems like a good idea. 👍🏽 |
Ok, updated the scip-go workflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed the vendored code, but let's make sure we add some comments here and in the monorepo to make it clear which code is coupled to which other code (e.g. using NOTE(id: blah)
which can be used for searching)
Mostly non-blocking/stylistic comments. Please @ mention me if you want me to take a deeper/second look at something specific.
cmd/src/code_intel_upload_flags.go
Outdated
@@ -53,6 +57,7 @@ var ( | |||
|
|||
func init() { | |||
codeintelUploadFlagSet.StringVar(&codeintelUploadFlags.file, "file", "", `The path to the SCIP index file.`) | |||
codeintelUploadFlagSet.BoolVar(&codeintelUploadFlags.gzipCompressed, "gzip", false, `Treat uploaded file as already gzip compressed`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this flag? Can we just do it purely based on the file extension without it being configurable?
cmd/src/code_intel_upload_flags.go
Outdated
return "", formatInferenceError(argumentInferenceError{ | ||
"file", | ||
errors.Newf("both %s and %s were found, choose the file to upload explicitly using -file flag", scipFilename, scipCompressedFilename), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not emit an error for ambiguity? I think the most likely situation is that the user compressed index.scip and created index.scip.gz without deleting the index.scip file. That doesn't warrant a hard error.
If both files are found, let's pick the compressed file and emit an info-level log message instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes GRAPH-1046
Contains a lot of code vendored in from the public snapshot – the main upload methods were unfortunately private so I had to unravel a bunch of dependencies.
Test plan