Allow setting the filename used in the protoc command #437

michaeldelago · 2024-06-23T19:01:29Z

This should resolve #431.

This PR makes it so setting PROTO-SOURCE-FILE specifically uses that value as the file name passed into protoc. With this change, lisp files generated from proto files will use the PROTO-SOURCE-FILE as the file_name template var that ultimately gets sent as a key to the *file-descriptors* variable.

For example, if you define a component in your asd file like (:proto-source-file "foo/bar/baz"), the resulting lisp file will contain these lines:

(cl:eval-when (:compile-toplevel :load-toplevel :execute)
(pi:add-file-descriptor #P"foo/bar/baz.proto" 'baz)
)

The upstream HEAD yields this form:

(cl:eval-when (:compile-toplevel :load-toplevel :execute)
(pi:add-file-descriptor #P"baz.proto" 'baz)
)

This is problematic because if another file contains import "foo/bar/baz.proto";, the asdf integration will fail (at validate-imports) since upstream HEAD adds #P"baz.proto" to *file-descriptors*.

Testing on SBCL 2.3.9 with Arch Linux (kernel version 6.9.3-arch1-1) passes.

additional changes

show output from protoc in case there's an error.
- This is useful as it can get unwieldy to copy/paste/enter the command that failed to see what went wrong while dialing in your asd file.
the well-known types have been updated to reflect the current upstream protobuf repo.
- This PR removes the need to change the import statement on google/protobuf/type.proto to only target source_context.proto.
- We could probably move this to a submodule, if desired.

michaeldelago · 2024-06-28T23:29:56Z

Thanks for kicking off the CI

Haven't had a reason to fiddle with ABCL, hopefully this is fun to fix

Slids · 2024-06-29T00:34:02Z

Ignore the ABCL test, it's been failing.

cgay

Just a few nits...

cgay · 2024-07-01T01:22:50Z

asdf.lisp

+(defun get-search-paths (protobuf-source-file)
+  "For a given protobuf-source-file, generate the search paths that should be used.
+To do this, it generates a search path from the component, as well as the
+PROTO-SEACH-PATH specified in the asd component.


PROTO-SEARCH-PATH or probably better as :proto-search-path ?

Done, thanks

cgay · 2024-07-01T01:24:17Z

asdf.lisp

@@ -108,6 +108,24 @@ to PARENT-PATH."
                (resolve-relative-pathname path parent-path))
              search-path))))

+(defun get-search-paths (protobuf-source-file)
+  "For a given protobuf-source-file, generate the search paths that should be used.


I think to pass internal Google linting rules the parameter name needs to be all caps, but I'm not sure about this. @Slids will know for sure.

Indeed, this will fail presubmits

resolved, thanks.

Are the linting rules public anywhere? Aware of the style guide but not sure if that's the same thing

Not that I know of. Maybe we can get @Slids to open source it. :-)

cgay · 2024-07-01T01:34:26Z

google/protobuf/descriptor.proto

@@ -645,6 +827,17 @@ message EnumOptions {

  reserved 5;  // javanano_as_lite

+  // Enable the legacy handling of JSON field name conflicts.  This lowercases
+  // and strips underscored from the fields before comparison in proto3 only.


underscored, haha. Silly Google. Jon, better get on this! :)

cgay · 2024-07-01T01:41:54Z

tests/deep-import-test.lisp

+                                       :signal-condition-on-fail t))
+
+(deftest test-all-imports-are-included (deep-import-suite)
+  "Ensure file imports of the parent structure are properly read and stored with it's file descriptor."


it's -> its

fixed, thanks

Slids · 2024-07-01T17:59:14Z

asdf.lisp

@@ -108,6 +108,24 @@ to PARENT-PATH."
                (resolve-relative-pathname path parent-path))
              search-path))))

+(defun get-search-paths (protobuf-source-file)
+  "For a given protobuf-source-file, generate the search paths that should be used.


Indeed, this will fail presubmits

Slids · 2024-07-01T18:00:40Z

cl-protobufs.asd

    :pathname ""
    :depends-on ("models" "misc")
    :components
-    ((:protobuf-source-file "descriptor"


you update the well known proto files, please do that in a seperate pull.

Done. Thanks

michaeldelago · 2024-07-03T13:51:19Z

Pretty sure I addressed all review notes, thanks for sending those.

Sorry for the delay, a lot going on this and next week

Slids

Ready for CI and internal review

Slids · 2024-09-21T03:21:56Z

internally we got these errors
CheckLint found errors.
Lines should be under 100 columns. (LINE-TOO-LONG)
//depot/google3/third_party/lisp/cl_protobufs/asdf.lisp:139
Lines should be under 100 columns. (LINE-TOO-LONG)
//depot/google3/third_party/lisp/cl_protobufs/asdf.lisp:143
Lines should be under 100 columns. (LINE-TOO-LONG)
//depot/google3/third_party/lisp/cl_protobufs/asdf.lisp:156
Lines should be under 100 columns. (LINE-TOO-LONG)
//depot/google3/third_party/lisp/cl_protobufs/asdf.lisp:160

Slids · 2024-09-26T21:30:27Z

Sorry it took so long, and thanks!

michaeldelago added 7 commits June 23, 2024 13:57

change how asdf runs protoc

6268c40

add tests

100beb3

typo

3ef4af8

add comment

f1f77be

get-search-paths: better doc string

65dd283

update well-known types

5d4c4b4

remove unnecessary search path

25acbbf

cgay reviewed Jul 1, 2024

View reviewed changes

Slids reviewed Jul 1, 2024

View reviewed changes

michaeldelago added 2 commits July 3, 2024 09:07

Address style review

5279a79

revert "update well-known types"

3e9fc7f

michaeldelago requested review from cgay and Slids July 3, 2024 13:49

michaeldelago added 2 commits July 3, 2024 09:56

add todo to update well-known types

e6b6b6c

Merge branch 'master' into proto-source-file-pathname

3bd948b

Slids approved these changes Sep 21, 2024

View reviewed changes

cgay approved these changes Sep 21, 2024

View reviewed changes

Slids added the Ready for Google label Sep 21, 2024

asdf.lisp: shorten line length

2c5b637

copybara-service bot merged commit 8540f07 into qitab:master Sep 26, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting the filename used in the protoc command #437

Allow setting the filename used in the protoc command #437

michaeldelago commented Jun 23, 2024

michaeldelago commented Jun 28, 2024

Slids commented Jun 29, 2024

cgay left a comment

cgay Jul 1, 2024

michaeldelago Jul 3, 2024

cgay Jul 1, 2024

Slids Jul 1, 2024

michaeldelago Jul 3, 2024

cgay Sep 21, 2024

cgay Jul 1, 2024

cgay Jul 1, 2024

michaeldelago Jul 3, 2024

Slids Jul 1, 2024

Slids Jul 1, 2024

michaeldelago Jul 3, 2024

michaeldelago commented Jul 3, 2024

Slids left a comment

Slids commented Sep 21, 2024

Slids commented Sep 26, 2024

Allow setting the filename used in the protoc command #437

Allow setting the filename used in the protoc command #437

Conversation

michaeldelago commented Jun 23, 2024

additional changes

michaeldelago commented Jun 28, 2024

Slids commented Jun 29, 2024

cgay 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeldelago commented Jul 3, 2024

Slids left a comment

Choose a reason for hiding this comment

Slids commented Sep 21, 2024

Slids commented Sep 26, 2024