Skip to content
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

Rework proto generation #1371

Merged
merged 2 commits into from
Dec 7, 2020
Merged

Rework proto generation #1371

merged 2 commits into from
Dec 7, 2020

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Nov 26, 2020

The changes here are:

  • Fix the default goal (using "default" target is not doing it).

  • Bail out with a useful message if proto submodule is not checked
    out.

  • Replace the use of docker image with downloading the protoc binary
    and building the gogofast plugin ourselves. This gives us a greater
    control over the invocation of protoc.

  • Use rsync to copy the generated code, instead of pax. Pax did not
    work for me (it was complaining about the unknown -0 flag).

The control over the protoc invocation will be useful later, when we
will want to generate a code with data structures in one place and the
collector code elsewhere. The collector code may or may not depend on
gRPC, but data structures have no need for it. This split will happen
when we move the gRPC code out of the OTLP exporter module into a
submodule.

Getting rid of docker has the upside that the generated files do not
belong to root, so there is no hassle of changing the ownership of the
files, and it is not requires to use sudo for the clean target. And
not using docker is faster.

The downside of this work is that it depends on more tools: rsync,
unzip and wget. I can only hope that macOS users have those tools too,
and that those tools are invoked the same.

@krnowak
Copy link
Member Author

krnowak commented Dec 1, 2020

Not adding a changelog entry for it, as it is nothing visible for users.

@evantorrie
Copy link
Contributor

The problem I did have before with some of these things is that the FreeBSD version of a utility is sometimes quite different from the GNU/Linux utility, particularly for command line flags.

The reason originally for choosing pax was that it has always been intended to be POSIX compatible across multiple OSes. What problems did you have running it?

Using docker for protoc was at the suggestion of some of the other language code contributors (e.g. Yuri from Uber), since it avoids cross-OS issues. Note that the opentelemetry-proto repo makes use of an Otel specific docker image which includes protoc. If we need the gogo plugin, I don't think it would be hard to add it to the docker image as it is built here: https://github.com/open-telemetry/build-tools/blob/e0cdd225af0479836d9418fc25d665d46c16f278/protobuf/README.md

Regardless, let me report back on whether this PR works ok on Darwin.

@krnowak
Copy link
Member Author

krnowak commented Dec 2, 2020

The problem I did have before with some of these things is that the FreeBSD version of a utility is sometimes quite different from the GNU/Linux utility, particularly for command line flags.

Yeah, that's painful. I can only hope that wget, rsync and unzip are used in the same way on FreeBSD and Linux.

The reason originally for choosing pax was that it has always been intended to be POSIX compatible across multiple OSes. What problems did you have running it?

It complained about the unknown -0 flag. Also, using pax (an archiver) to copy over files feels like a hack. Clever, but opaque. :)

Using docker for protoc was at the suggestion of some of the other language code contributors (e.g. Yuri from Uber), since it avoids cross-OS issues. Note that the opentelemetry-proto repo makes use of an Otel specific docker image which includes protoc. If we need the gogo plugin, I don't think it would be hard to add it to the docker image as it is built here: https://github.com/open-telemetry/build-tools/blob/e0cdd225af0479836d9418fc25d665d46c16f278/protobuf/README.md

Regardless, let me report back on whether this PR works ok on Darwin.

I'd prefer to avoid using docker for reasons mentioned in the PR message.

Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I tried this PR on a Mac, and it works fine except for the missing \ at the end of the shell script lines.

Understood on the more control needed for protobuf generation.

Makefile.proto Outdated Show resolved Hide resolved
The changes here are:

- Fix the default goal (using "default" target is not doing it).

- Bail out with a useful message if proto submodule is not checked
  out.

- Replace the use of docker image with downloading the protoc binary
  and building the gogofast plugin ourselves. This gives us a greater
  control over the invocation of protoc.

- Use rsync to copy the generated code, instead of pax. Pax did not
  work for me (it was complaining about the unknown -0 flag).

The control over the protoc invocation will be useful later, when we
will want to generate a code with data structures in one place and the
collector code elsewhere. The collector code may or may not depend on
gRPC, but data structures have no need for it. This split will happen
when we move the gRPC code out of the OTLP exporter module into a
submodule.

Getting rid of docker has the upside that the generated files do not
belong to root, so there is no hassle of changing the ownership of the
files, and it is not requires to use sudo for the `clean` target. And
not using docker is faster.

The downside of this work is that it depends on more tools: rsync,
unzip and wget. I can only hope that macOS users have those tools too,
and that those tools are invoked the same.
@krnowak
Copy link
Member Author

krnowak commented Dec 4, 2020

Updated to fix the protogen github workflow (it needs to install different tools now).

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #1371 (79702a8) into master (787e3f4) will increase coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1371   +/-   ##
======================================
  Coverage    77.5%   77.6%           
======================================
  Files         124     124           
  Lines        6126    6126           
======================================
+ Hits         4753    4755    +2     
+ Misses       1122    1120    -2     
  Partials      251     251           
Impacted Files Coverage Δ
exporters/otlp/connection.go 85.3% <0.0%> (+1.8%) ⬆️

@MrAlias MrAlias added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 7, 2020
@MrAlias MrAlias merged commit 0021ab0 into open-telemetry:master Dec 7, 2020
@krnowak krnowak deleted the proto-gen branch December 12, 2023 14:54
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants