Skip to content

Improve and fix support for compiling protobuf files#19

Merged
leandro-lucarella-frequenz merged 4 commits into
frequenz-floss:mainfrom
llucax:api
May 8, 2023
Merged

Improve and fix support for compiling protobuf files#19
leandro-lucarella-frequenz merged 4 commits into
frequenz-floss:mainfrom
llucax:api

Conversation

@leandro-lucarella-frequenz
Copy link
Copy Markdown
Contributor

The main changes are:

  • Avoid running sessions on sources for API repos

    API repos are supposed to just ship python files generated from protocol buffer files. Only a stub __init__.py should be included, which doesn't really need to be checked.

    Allowing checks on the sources for an API repos triggers a lot of issues when the protocol buffers files are generated, as they don't pass any checks. An alternative would be to exclude auto-generated files, but it's not easy because each tool has its own exclusion mechanism, and some don't even have any (hello darglint), so it is simpler to just skip testing the stub __init__.py file.

  • Avoid the need to write a setup.py file for API repos

    To do this we declare a distutils entry point in the pyproject.toml file to automatically add the compile_proto command using the CompileProto proto class.

    Then, in the grpc_tools module we just add the newly declared command as a build sub-command, so it is executed first. This make all other subsequent sub-commands will see the generated sources as if they were already there.

    The only downside of this is now we don't have an easy way to configure the command. The options are there, and can be passed via command-line, like:

    python -c 'import setuptools; setuptools.setup()' compile_proto \
        --proto-path=another/path

    But there is no easy way to do it in a config file, or at least at the moment I couldn't find a way to pass arbitrary options to setuptools commands in pyproject.toml. If we need this, we could manually parse the pyproject.toml file and look for options, just like setuptools_scm does.

llucax added 4 commits April 12, 2023 11:46
Signed-off-by: Leandro Lucarella <leandro.lucarella@frequenz.com>
This can prevent some issues with editable installs and mypy, which
sometimes complains about a frequenz.xxx module not having the py.typed
file.

Usually this file is created automatically by pip when doing proper
installs, but when using editable installs that doesn't happen.

Signed-off-by: Leandro Lucarella <leandro.lucarella@frequenz.com>
API repos are supposed to just ship python files generated from
protocol buffer files. Only a stub __init__.py should be included,
which doesn't really need to be checked.

Allowing checks on the sources for an API repos triggers a lot of issues
when the protocol buffers files are generated, as they don't pass any
checks. An alternative would be to exclude auto-generated files, but
it's not easy because each tool has its own exclusion mechanism, and
some don't even have any (hello darglint), so it is simpler to just skip
testing the stub __init__.py file.

Signed-off-by: Leandro Lucarella <leandro.lucarella@frequenz.com>
To do this we declare a distutils entry point in the pyproject.toml file
to automatically add the `compile_proto` command using the
`CompileProto` proto class.

Then, in the `grpc_tools` module we just add the newly declared command
as a `build` sub-command, so it is executed first. This make all other
subsequent sub-commands will see the generated sources as if they were
already there.

The only downside of this is now we don't have an easy way to configure
the command. The options are there, and can be passed via command-line
(like:

```sh
python -c 'import setuptools; setuptools.setup()' compile_proto \
    --proto-path=another/path
```

But there is no easy way to do it in a config file, or at least at the
moment I couldn't find a way to pass arbitrary options to setuptools
commands in `pyproject.toml`. If we need this, we could manually parse
the `pyproject.toml` file and look for options, just like
`setuptools_scm` does.

Signed-off-by: Leandro Lucarella <leandro.lucarella@frequenz.com>
@leandro-lucarella-frequenz leandro-lucarella-frequenz added this to the v0.1.0 milestone Apr 13, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz added type:bug Something isn't working type:enhancement New feature or enhancement visitble to users part:protobuf Affects the protobuf tools labels Apr 13, 2023
@leandro-lucarella-frequenz
Copy link
Copy Markdown
Contributor Author

Incidentally, while looking at the microgrid API repo to see how it was doing this, I realized the CI is not really testing anything. It uses tox and it is run without specifying a session, which means tox runs some py sessions that doesn't do anything 🎉 🤦

@leandro-lucarella-frequenz
Copy link
Copy Markdown
Contributor Author

With this I was able to migrate the dispatch API repo to use this one.

@leandro-lucarella-frequenz
Copy link
Copy Markdown
Contributor Author

OK, no complains for a month now, so force-merging 💪

@leandro-lucarella-frequenz leandro-lucarella-frequenz merged commit c3239e4 into frequenz-floss:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:protobuf Affects the protobuf tools type:bug Something isn't working type:enhancement New feature or enhancement visitble to users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants