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.

Sign up for GitHub

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

Jump to bottom

Upgrade to tonic 0.8.x #506

Merged
merged 4 commits into from
Mar 20, 2023
Merged

Upgrade to tonic 0.8.x #506

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
956deec
Upgrade to tonic 0.8.x
sophokles73 Mar 10, 2023
753f633
Incorporate feedback
sophokles73 Mar 17, 2023
d478a20
Disable default features in top level Cargo.toml
sophokles73 Mar 17, 2023
389287c
Install protoc via apt-get install
sophokles73 Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/kuksa_databroker-cli_build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ jobs:
runs-on: ubuntu-latest

steps:
- name: Install Protoc
uses: arduino/setup-protoc@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just apt install it?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would, of course, also work but then you have no control over the version being installed (which may or may not be relevant). If you'd rather have the OS package being used, I will be happy to change it ...

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point as well.

Preferably though, we want it to work with any protoc version (which by the way probably means we should rethink the current use of optional in the kuksa.val.v1 .proto files).

I would say if it just works replacing it with apt install at this point in time, I'd go with that.

with:
version: '3.x'
- uses: actions/checkout@v3
- name: cargo fmt
working-directory: ${{github.workspace}}
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/kuksa_databroker_build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ jobs:
runs-on: ubuntu-latest

steps:
- name: Install Protoc
uses: arduino/setup-protoc@v1
with:
version: '3.x'
- uses: actions/checkout@v3
- name: cargo fmt
working-directory: ${{github.workspace}}
Expand All @@ -45,6 +49,10 @@ jobs:
runs-on: ubuntu-latest

steps:
- name: Install Protoc
uses: arduino/setup-protoc@v1
with:
version: '3.x'
- uses: actions/checkout@v3
- uses: actions-rs/[email protected]
with:
Expand Down
169 changes: 121 additions & 48 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,24 @@ members = [
"kuksa_databroker/databroker-examples",
]

[workspace.dependencies]
clap = { version = "3.1.10", default-features = false, features = [
"std",
"env",
] }
databroker-proto = { path = "kuksa_databroker/databroker-proto" }
prost = "0.11"
prost-types = "0.11"
tokio = { version = "1.17.0", features = [
"macros",
"rt-multi-thread",
"time",
"signal",
] }
tokio-stream = { version = "0.1.8", features = ["sync", "net"] }
tonic = "0.8"
tonic-build = "0.8"

Copy link
Contributor

Choose a reason for hiding this comment

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

Good initiative with workspace dependencies!

However, I think it would be better to use it only for specifying a shared version, i.e. (tokio = { version = "1.17.0" }), and not features. Features are additive anyway, and it's easier to keep track of which crate needs which feature if it's added for each crate instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and one more thing. Since features are additive, in order for the workspace.dependencies not to inadvertently enable all the default features (see default-features ignored), default-features = false should also be retained in the workspace dependencies.

So at least for clap in this case (and maybe for all?)

clap = { version = "3.1.10", default-features = false }

Copy link
Member Author

@sophokles73 sophokles73 Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't that also be the decision (responsibility) of the different packages? In the end they will need to know if they need/want the default features or not, or won't they?

Copy link
Contributor

@argerus argerus Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes exactly. So in order to give them the ability to make that decision, the workspace dependencies need to have default-features = false.

Otherwise, default-features will be enabled for every member regardless of what they specify themself.

[profile.release]
lto = true # Link time optimization (dead code removal etc...)
opt-level = "s"
Expand Down
Loading