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

Add daita as a Cargo cfg variable #6231

Merged
merged 46 commits into from
May 17, 2024

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented May 7, 2024

Due to reasons outlined here #6202 (review), It may be problematic to gate DAITA behind a cargo feature. This is an alternative approach which avoids the use of features.


This change is Reviewable

@Serock3 Serock3 self-assigned this May 7, 2024
@Serock3 Serock3 changed the base branch from main to wireguard-go-rs May 7, 2024 10:38
@Serock3 Serock3 changed the base branch from wireguard-go-rs to add-daita-cfg-flag May 8, 2024 07:57
.github/workflows/cargo-vendor.yml Outdated Show resolved Hide resolved
wireguard-go-rs/build.rs Outdated Show resolved Hide resolved
@Serock3 Serock3 force-pushed the add-daita-cfg-flag-no-feature branch from cade6d7 to 82855d4 Compare May 8, 2024 12:21
@Serock3 Serock3 force-pushed the add-daita-cfg-flag branch from daf0938 to f14fd21 Compare May 8, 2024 14:32
@Serock3 Serock3 force-pushed the add-daita-cfg-flag-no-feature branch 2 times, most recently from 0ea1234 to d5be080 Compare May 8, 2024 14:52
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.

Reviewed 29 of 53 files at r1, 23 of 33 files at r3, 8 of 10 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 61 of 64 files reviewed, 3 unresolved discussions (waiting on @hulthe and @Serock3)


build.sh line 168 at r3 (raw file):

if [[ "$(uname -s)" == "MINGW"* || "$(uname -s)" == "Linux" ]]; then
    CARGO_ARGS+=(--features daita)
fi

This should be removed, otherwise build.sh will fail when compiling due to the daita feature not existing on Linux and Windows 😊

Code quote:

# Enable DAITA on supported platforms
if [[ "$(uname -s)" == "MINGW"* || "$(uname -s)" == "Linux" ]]; then
    CARGO_ARGS+=(--features daita)
fi

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r2, 2 of 33 files at r3, all commit messages.
Reviewable status: 63 of 64 files reviewed, 3 unresolved discussions (waiting on @Serock3)


.github/workflows/cargo-vendor.yml line 24 at r2 (raw file):

        run: |
          git config --global --add safe.directory '*'
          git submodule update --init --depth=1 dist-assets/binaries wireguard-go-rs/libwg

shouldn't this be wireguard-go-rs/libwg/wireguard-go?


.github/workflows/desktop-e2e.yml line 164 at r5 (raw file):

        uses: actions/checkout@v2
      - name: Checkout submodules
       # TODO: should this specify a path? We don't want to pull `wireguard-go`

better to have it and not need it? 🤷
it's probably a non-issue to just keep this

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag-no-feature branch 5 times, most recently from dc84098 to 710c4bc Compare May 14, 2024 12:43
@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag-no-feature branch from 710c4bc to 6f1e290 Compare May 14, 2024 12:49
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review May 14, 2024 12:50
@Serock3 Serock3 force-pushed the add-daita-cfg-flag-no-feature branch from 59c389f to a540b5e Compare May 15, 2024 07:28
@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag-no-feature branch from a540b5e to c0bf274 Compare May 15, 2024 07:33
MarkusPettersson98 and others added 14 commits May 15, 2024 09:36
Replace `cargo:key=value` with `cargo::key=value`
Instead of gating `daita` per platform in the `build-wireguard-go.sh`,
accept an argument which says whether DAITA support should be enabled.
Include `maybenot` as a submodule of our `wireguard-go` fork with
DAITA-support. Use this submodule to build the `maybenot-ffi` on
platforms which should build `wireguard-go` with DAITA (not windows).

The main benefit of doing this is that Windows target no longer needs
to clone the `wireguard-go` submodule.
@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag-no-feature branch from c0bf274 to 00f210d Compare May 15, 2024 07:36
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.

Reviewed 1 of 5 files at r9.
Reviewable status: 61 of 78 files reviewed, 4 unresolved discussions (waiting on @dlon and @hulthe)


.github/workflows/desktop-e2e.yml line 164 at r5 (raw file):

Previously, dlon (David Lönnhager) wrote…

+1

I agree


.github/workflows/rust-unused-dependencies.yml line 92 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

The macOS and Windows jobs can probably be merged if just the Install go step is skipped on Windows.

Done


wireguard-go-rs/libwg/libwg.a line at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

This should probably be added to .gitignore? And completely removed from the git history as well (so it's not needlessly fetched).

Done (I think). Please verify that it is actually completely nuked from the git history 😊

@MarkusPettersson98 MarkusPettersson98 force-pushed the add-daita-cfg-flag-no-feature branch from 4ebea97 to 6af681a Compare May 15, 2024 11:36
@Serock3 Serock3 merged commit aa19a28 into add-daita-cfg-flag May 17, 2024
47 of 52 checks passed
@Serock3 Serock3 deleted the add-daita-cfg-flag-no-feature branch May 17, 2024 13:57
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.

4 participants