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

chore(cargo workspace): unifies crates as workspace #227

Conversation

EandrewJones
Copy link
Contributor

Changes

Combines all rust crates under a single workspace to simplify management of rust packaging.

Note: All crates are converted to the workspace with the exception of ebpf which needs independent tooling and packages to properly build.

Closes #218

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2024
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 19, 2024
@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch from 38ca8d3 to 0a881f0 Compare April 19, 2024 02:50
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024
@shaneutt shaneutt requested a review from astoycos April 19, 2024 11:35
@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch 2 times, most recently from 6bd43c3 to 0347bcc Compare April 19, 2024 17:05
@EandrewJones
Copy link
Contributor Author

EandrewJones commented Apr 19, 2024

Well I wanted to punt on touching the Makefile and Docker builds, but looks like I may have to anyways since the workspace breaks docker image builds. And if I'm going to do it, I may as well do it the right way.

@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch from 0347bcc to c4267a4 Compare April 29, 2024 00:15
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 29, 2024
@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch from c4267a4 to 54bb634 Compare April 29, 2024 00:41
@EandrewJones
Copy link
Contributor Author

@astoycos Ready for your review. Apologies for the large PR. Unfortunately, I also had to update the CI, Makefiles, and Dockerfiles to get the test suite passing with the transition to a workspace.

@astoycos
Copy link
Member

astoycos commented May 6, 2024

No worries I'm a bit backed up on the TODO list but I will get to this as soon as I can

service.controlplane.Dockerfile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@@ -21,7 +26,11 @@ RUN if [ "$TARGETARCH" = "amd64" ]; \
fi
RUN rustup target add $(eval cat arch)-unknown-linux-musl

COPY . .
COPY dataplane dataplane
COPY tools/udp-test-server tools/udp-test-server
Copy link
Member

Choose a reason for hiding this comment

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

do we need this in the dataplane?

Copy link
Contributor Author

@EandrewJones EandrewJones May 31, 2024

Choose a reason for hiding this comment

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

Unfortunately, we do. If I recall correctly, cargo complains if the referenced workspace members are missing, even if you're only building a single package.

I don't think this is that big of a deal, however, because it doesn't get copied over to the second stage.

If you know a work around, please share.

service.dataplane.Dockerfile Outdated Show resolved Hide resolved
@EandrewJones
Copy link
Contributor Author

EandrewJones commented May 30, 2024 via email

Copy link
Member

@astoycos astoycos left a comment

Choose a reason for hiding this comment

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

Just some small things, additionally can you remove https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/ebpf/Cargo.toml#L36 from the ebpf crate's cargo toml it's not a workspace just a standalone crate

You could also document somewhere that the ebpf crate can't be part of the workspace because

error[E0152]: found duplicate lang item `panic_impl`
   --> dataplane/ebpf/src/main.rs:117:1
    |
117 | / fn panic(_info: &core::panic::PanicInfo) -> ! {
118 | |     loop {}
119 | | }
    | |_^

Or more specifically because the crate needs to re-implements the panic handler

service.controlplane.Dockerfile Outdated Show resolved Hide resolved
service.dataplane.Dockerfile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch 2 times, most recently from 3899eb0 to 7f967e6 Compare May 31, 2024 01:47
@EandrewJones
Copy link
Contributor Author

Just some small things, additionally can you remove https://github.com/kubernetes-sigs/blixt/blob/main/dataplane/ebpf/Cargo.toml#L36 from the ebpf crate's cargo toml it's not a workspace just a standalone crate

You could also document somewhere that the ebpf crate can't be part of the workspace because

error[E0152]: found duplicate lang item `panic_impl`
   --> dataplane/ebpf/src/main.rs:117:1
    |
117 | / fn panic(_info: &core::panic::PanicInfo) -> ! {
118 | |     loop {}
119 | | }
    | |_^

Or more specifically because the crate needs to re-implements the panic handler

I added a note in the README.

Combines all rust crates under a single workspace
to simplify management of rust packaging, with
the exception of ebpf which needs independent
tooling and packages to properly build.

Author: Evan Jones <[email protected]>
@EandrewJones EandrewJones force-pushed the #218-convert-repo-to-rust-workspace branch from 7f967e6 to 8a8157a Compare May 31, 2024 01:53
@EandrewJones EandrewJones requested a review from astoycos May 31, 2024 02:05
@astoycos
Copy link
Member

/lgtm
/approve

You're the man @EandrewJones!!! Sorry for my slowness on this one

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astoycos, EandrewJones

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 2df33c4 into kubernetes-sigs:main May 31, 2024
12 checks passed
@EandrewJones
Copy link
Contributor Author

/lgtm /approve

You're the man @EandrewJones!!! Sorry for my slowness on this one

No worries. I'm quite buried myself right now, so I appreciated the slowness ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert our repo to be a rust workspace
4 participants