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

Take control over maybenot dependencies and verification #7515

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

Conversation

faern
Copy link
Member

@faern faern commented Jan 24, 2025

This PR is a sibling-PR to mullvad/wireguard-go#22.

This switches to a version of the wireguard-go submodule where maybenot-ffi is not included and built as a submodule. Instead there is a local crate in wireguard-go that acts as a simple wrapper around maybenot-ffi, with helper files to build it for static linking. This allows us to include the wrapper (and transitively all its dependencies) in our main workspace and track all dependency versions and checksums here. This simplifies dependency management, reproducible builds and supply chain security checks.

This PR is going to be colliding a bit with DAITA v2 for Windows (#7457), we have to discuss which one to get merged first.

Fixes DES-1360 and DES-1645


This change is Reviewable

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

I like it. Solves the lockfile situation and makes static vs dynamic linking less messy. 👍

Reviewed 8 of 9 files at r1.
Reviewable status: 8 of 10 files reviewed, all discussions resolved

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 9 files at r1.
Reviewable status: 8 of 10 files reviewed, all discussions resolved

@albin-mullvad albin-mullvad requested a review from kl January 24, 2025 12:14
kl
kl previously approved these changes Jan 24, 2025
Copy link
Contributor

@kl kl left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

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

There are still some references to git submodule update --init --recursive --depth=1 wireguard-go-rs (for example in the top-level README.md). We should no longer need to checkout recursive submodules in wireguard-go-rs 😊

Reviewed 8 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad
Copy link
Collaborator

albin-mullvad commented Jan 24, 2025

Do we need to pass --locked to the cargo build command in the maybenot-ffi Makefile since we currently don't seem to do so? Or somehow address it in another way?

https://github.com/maybenot-io/maybenot/blob/5f1389fd81523bb686f3054c460e4365e10527db/crates/maybenot-ffi/Makefile#L32

EDIT: Sorry, never mind, seems to be handled in the related wireguard-go PR.

@faern faern force-pushed the take-control-over-maybenot-dependencies-and-verification-des-1645 branch from 8105490 to 6a87580 Compare February 6, 2025 13:09
@faern faern force-pushed the take-control-over-maybenot-dependencies-and-verification-des-1645 branch from 60029cc to b661f25 Compare February 6, 2025 13:25
@faern faern force-pushed the take-control-over-maybenot-dependencies-and-verification-des-1645 branch 5 times, most recently from 2a55782 to 8947760 Compare February 6, 2025 14:59
@faern faern force-pushed the take-control-over-maybenot-dependencies-and-verification-des-1645 branch from 8947760 to b5da9be Compare February 6, 2025 15:12
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.

6 participants