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

Switched protobuffer build script to new lib #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KamyaPA
Copy link

@KamyaPA KamyaPA commented Nov 3, 2023

Removed submodule Ecdar-Protobuf
Removed build.rs
Added dependency Ecdar-Protobuf-rs

@KamyaPA KamyaPA linked an issue Nov 3, 2023 that may be closed by this pull request
…-from-build-script-to-lib-group5

# Conflicts:
#	Ecdar-ProtoBuf
@Aavild
Copy link

Aavild commented Dec 13, 2023

fixed merge conflict

@Aavild Aavild requested a review from williamwoldum December 13, 2023 12:58
@williamwoldum
Copy link

williamwoldum commented Dec 13, 2023

Maybe I've missed something, and let me know if thats the case

Removing the submodule pointer from Reveaal seriously harms the flexibility of protobuf development/implementation.

With this PR, if i wanted to work with a different Ecdar-Protobuf branch in Reveaal I would have to fork Ecdar-Protobuf-rs, update the submodule pointer, and update Cargo.toml to the forked repo.

While this might be okay for Reveaal where the protobuf services are fairly stable, it would be a major pain when working with the Ecdar-API where new services and messages are added frequently.

If Ecdar-Protobuf-rs does not work as a common protobuf-build-solution for all repos, and harms development, Im not sure i see the benefit of using it.

@Aavild
Copy link

Aavild commented Dec 13, 2023

I think you may be right. Also given that we want yet another submodule pointer in #19

@KamyaPA KamyaPA closed this Dec 13, 2023
@KamyaPA KamyaPA reopened this Dec 13, 2023
@KamyaPA
Copy link
Author

KamyaPA commented Dec 13, 2023

Maybe I've missed something, and let me know if thats the case

Removing the submodule pointer from Reveaal seriously harms the flexibility of protobuf development/implementation.

With this PR, if i wanted to work with a different Ecdar-Protobuf branch in Reveaal I would have to fork Ecdar-Protobuf-rs, update the submodule pointer, and update Cargo.toml to the forked repo.

While this might be okay for Reveaal where the protobuf services are fairly stable, it would be a major pain when working with the Ecdar-API where new services and messages are added frequently.

If Ecdar-Protobuf-rs does not work as a common protobuf-build-solution for all repos, and harms development, Im not sure i see the benefit of using it.

I've added this functionality in Ecdar-Protobuff-rs such that you can compile with any root folder by setting an environment variable

@williamwoldum
Copy link

williamwoldum commented Dec 13, 2023

I've added this functionality in Ecdar-Protobuff-rs such that you can compile with any root folder by setting an environment variable

I see how this works around the issue, but we are over-complicating things.

I don't really want to download Ecdar-ProtoBuf next to Reveaal and then configure the absolute path in Ecdar-Protobuff-rs.

I think Ecdar-Protobuff-rs is trying to solve a problem that does not exist.

Sure, it provides a common build.rs file and a common serde macro, but even build.rs should not necessarily be the same for every repo. For Ecdar-API we might want to add attributes via build.rs that are not relevant for Reveaal.

Further if i wanted to add new stuff to the common build.rs during development, i would still have to fork Ecdar-Protobuff-rs and update Cargo.toml to apply any changes.

✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change protobuffer from build script to lib
5 participants