Skip to content
This repository has been archived by the owner on Jun 3, 2022. It is now read-only.

merge related vpp-api-gen crates into workspace repo #25

Open
pepinns opened this issue Nov 30, 2021 · 7 comments
Open

merge related vpp-api-gen crates into workspace repo #25

pepinns opened this issue Nov 30, 2021 · 7 comments

Comments

@pepinns
Copy link
Contributor

pepinns commented Nov 30, 2021

How do you feel about merging all these dependent repos into 1 and using cargo workspaces to build them? https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html

You can still reference individual crates at different versions if you want, but in practice you need all these at the same version almost always, and it makes submitting cross-crate PRs a 1 step process for testing and reviewing.

You should be able to use a git subtree to merge the existing repos and save commit history as well. Happy to take a stab at it in this or a fresh repo if you agree.

Example is tokio. https://github.com/tokio-rs/tokio/blob/master/Cargo.toml
All of the crates are just sub-directories of the tokio-rs repo.

@ayourtch
Copy link
Owner

ayourtch commented Dec 2, 2021

Hi Jim,

yeah this is an interesting idea, my only uncertainty is with the generated code… see e.g. “latest-vpp-api” repo which is rebuilt on a daily basis and aims to be tracking the latest changes: https://github.com/ayourtch/latest-vpp-api

It refers to individual crates via their git urls - will it be possible to retain that ? (I assume just specifying path one level deeper will work?)

My idea was that one can have several versions of the api bindings done against different VPP versions and potentially use them in parallel… (via a higher level abstraction crate that is TBD)

Will that still work ?

If yes - I have made early on a “vpp-api” placeholder repo which we could use as a place to consolidate the other ones.

@pepinns
Copy link
Contributor Author

pepinns commented Dec 2, 2021

Yes. All that still works.

Example of using the tracing lib with multiple crates in one repo:

tracing = { git = "https://github.com/tokio-rs/tracing.git", branch = "master" 
tracing-subscriber = { git = "https://github.com/tokio-rs/tracing.git", branch = "master" }
tracing-tower = { git = "https://github.com/tokio-rs/tracing.git", branch = "master"}
tracing-core = { git = "https://github.com/tokio-rs/tracing.git", branch = "master" }

cargo is smart enough to know to look for tracing-tower at tracing.git/tracing-tower, at the branch you specify.

latest-vpp-api seems like it should still be separate as that tracks the VPP version, and not really the version of these crates. There is no real coupling between the 'master' version of vpp-api-gen, and the 'master' version of latest-vpp-api like there is between vpp-api-gen and vpp-api-transport for example.

The generated messages are ephemeral in the sense that they need to be re-generated when you update your version of the codegen software, or when you update your version of VPP. Putting it up on a crate is nice because it makes it easier for first-timers to get coding right away on the latest stuff, but anyone using this for a project is likely to generate their own messages from their version(s) of VPP, possibly even including those generated messages into their own project like they would with protobuf or openapi generated code. When they update these libs, they'll likely also re-generate the messages for their vetted VPP version.

Since its generated it doesn't have the same development workflow overhead either. ie. no PRs to review/test across multiple versions. Based on what I see with the github actions workflow, its basically hands off and just updates itself every day.

I think the main things you get are

  • developer/reviewer productivity
    • doesn't rely on people making good commit/PR messages to see what code was introduced together.
    • easier to validate changes that cut across these deps by checking out one repo and running build/tests
  • single-version for all these highly related dependencies
    • simplified dependency mgmt ( can put relative paths in Cargo.toml inside the repo )
  • cargo build/test everything at once from the root

And you can still pick/choose which versions you use separately if you want, but inside the workspace repo you can punt all that by referencing local paths like so: https://github.com/tokio-rs/tracing/blob/master/tracing-tower/Cargo.toml#L28 so all your crates are always tested against the one single version of the group, and they all move forward together.

Practically though I can't see anyone wanting to mix and match versions of these, unless they have custom changes to one that they don't want to push upstream for some reason.

RE: higher level api
I think thats a great idea, and I see no reason that couldn't also live in the same workspace repo.

Would you like me to setup a PR then against vpp-api?

@ayourtch
Copy link
Owner

ayourtch commented Dec 2, 2021

cool! so yeah i think this is precisely the kind of usage i thought for vpp-api repo for, so i will be happy for your PR! thanks ! :)

@ayourtch
Copy link
Owner

ayourtch commented Dec 2, 2021

(and thank you so much for such a detailed comment btw - I am terse because roaming with iphone).

@ayourtch
Copy link
Owner

ayourtch commented Dec 2, 2021

by the way - i tried a somewhat involved api use in https://github.com/ayourtch/vpp-bonjour and it feels like i made compiler quite unhappy…. so i think there’s a few more kinks that will need ironing out… and docs… i will probably take a first stab at it when we merge it all into the project repo…

@pepinns
Copy link
Contributor Author

pepinns commented Dec 2, 2021

ayourtch/vpp-api#1 The files changed is huge, but if you look at the most recent commits you'll see its just git subtree add --prefix vpp-api-encoding [email protected]:ayourtch/vpp-api-encoding.git main for each of the repos, and then modify the main Cargo.toml to include all the directories.

@ayourtch
Copy link
Owner

ayourtch commented Dec 2, 2021

yeah looks reasonable! i merged it, thanks for taking care of it! i must say it does look even aesthetically pleasing :)

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

No branches or pull requests

2 participants