qitab / cl-protobufs

Common Lisp protocol buffer implementation.
MIT License
84 stars 18 forks source link

Simplify compilation process #192

Closed bkuehnert closed 3 years ago

bkuehnert commented 4 years ago

This commit should make the build process simple enough for quicklisp.

If protoc and protoc-gen-lisp are available on the system, they will be used to generate & load the schemas for the well known types, as usual.

If not, the .wkt subdirectory contains pre-generated Lisp schemas for the well known types that will allow :cl-protobufs to build.

This PR also adds a github action which will trigger on any push that'll re-generate these schemas and commit them if there are any changes.

sionescu commented 4 years ago

With this approach, when the .asd file is evaluated the first time :protoc will not be present in *features* and the bundled definitions will always be loaded. In a batch compilation scenario (as opposed to an interactive programming session) protoc will never be used for cl-protobufs itself because the .asd file will not be re-evaluated.

What happens when there's a skew between the protoc version used to generate the bundled files and the one on the user's machine ? If you can assume that the well-known type definitions are stable across protoc releases then you could simply avoid the whole auto-detection and always load the bundled definitions. If not, perhaps the easiest would be to add a version check on the protoc compiler and refuse to work with versions outside a safe interval.

bkuehnert commented 4 years ago

With this approach, when the .asd file is evaluated the first time :protoc will not be present in features and the bundled definitions will always be loaded.

The intention is that the :protoc feature will be active if and only if the machine can run protoc and protoc-gen-lisp. The bundled definitions exist as a fall back if and only if we can't generate the lisp files. Is this not the behavior of the .asd file currently? If not, that's certainly a bug.

In a batch compilation scenario (as opposed to an interactive programming session) protoc will never be used for cl-protobufs itself because the .asd file will not be re-evaluated.

I'm not familiar with batch compilation means here, but if the .asd file is run once (and protoc is installed at that time) then protoc should always be used, no?

What happens when there's a skew between the protoc version used to generate the bundled files and the one on the user's machine ?

If there's any version of protoc installed on the user's machine, then that one will be used to generate the .lisp files. When code is pushed to github, the bundled lisp files will be updated (using the protoc version at head to generate the files).

If you can assume that the well-known type definitions are stable across protoc releases then you could simply avoid the whole auto-detection and always load the bundled definitions. If not, perhaps the easiest would be to add a version check on the protoc compiler and refuse to work with versions outside a safe interval.

That is a safe assumption. Sadly, it's not sufficient since there are wild changes with the .proto -> .lisp conversion. The problem is not the version of protoc, it's the changes in cl-protobufs itself.

sionescu commented 4 years ago

With this approach, when the .asd file is evaluated the first time :protoc will not be present in features and the bundled definitions will always be loaded.

The intention is that the :protoc feature will be active if and only if the machine can run protoc and protoc-gen-lisp. The bundled definitions exist as a fall back if and only if we can't generate the lisp files. Is this not the behavior of the .asd file currently? If not, that's certainly a bug.

The reader "#+protoc" macro in cl-protobufs.asd will be evaluated (parsed) before the defmethod that checks for protoc is even compiled. I get what you're trying to do there but that's not the current behaviour of the .asd file.

In a batch compilation scenario (as opposed to an interactive programming session) protoc will never be used for cl-protobufs itself because the .asd file will not be re-evaluated.

I'm not familiar with batch compilation means here, but if the .asd file is run once (and protoc is installed at that time) then protoc should always be used, no?

I mean when compiling from the command line (in batch mode).

What happens when there's a skew between the protoc version used to generate the bundled files and the one on the user's machine ?

If there's any version of protoc installed on the user's machine, then that one will be used to generate the .lisp files. When code is pushed to github, the bundled lisp files will be updated (using the protoc version at head to generate the files).

If you can assume that the well-known type definitions are stable across protoc releases then you could simply avoid the whole auto-detection and always load the bundled definitions. If not, perhaps the easiest would be to add a version check on the protoc compiler and refuse to work with versions outside a safe interval.

That is a safe assumption. Sadly, it's not sufficient since there are wild changes with the .proto -> .lisp conversion. The problem is not the version of protoc, it's the changes in cl-protobufs itself.

In that case I don't understand what this commit improves. Is it just so that Xach be able to compile cl-protobufs without installing protoc ? Why is that different from any other Lisp library that wraps a C library ? Since cl-protobufs cannot be meaningfully used without protoc (who uses protobuf without a custom interface ?), making one very specific use case more convenient doesn't seem worth the trouble, as this commit makes the compilation process more complex, not simpler.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/qitab/cl-protobufs/pull/192#issuecomment-699650212, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABO6OIIVQJHTRBX6WZRCB3SH5LSJANCNFSM4QNM6PYQ.

-- Stelian Ionescu

bkuehnert commented 4 years ago

The reader "#+protoc" macro in cl-protobufs.asd will be evaluated (parsed) before the defmethod that checks for protoc is even compiled. I get what you're trying to do there but that's not the current behaviour of the .asd file.

Ah, I see now. Thank you!

In that case I don't understand what this commit improves. Is it just so that Xach be able to compile cl-protobufs without installing protoc ? Why is that different from any other Lisp library that wraps a C library ? Since cl-protobufs cannot be meaningfully used without protoc (who uses protobuf without a custom interface ?), making one very specific use case more convenient doesn't seem worth the trouble, as this commit makes the compilation process more complex, not simpler.

Yes, this change is purely so that the cl-protobufs can be loaded without using anything non-lisp. Part of quicklisp's criteria is that the build process cannot require any action outside of ASDF.

I agree that it seems a bit silly to put it on quicklisp, since the user will still need to download the repo anyway in order to download/install the protoc plugin. Any thoughts @Slids ?