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 rustc remap prefix helper crate #7508

Closed
wants to merge 2 commits into from

Conversation

kl
Copy link
Contributor

@kl kl commented Jan 23, 2025

Fixes: DROID-1747

This is needed for reproducible builds. This binary helper crate gets the file paths to the user's Cargo and Rustup home dirs, as well as the path to the Mullvad app source dir. These paths are then remapped to fixed values which is needed to make the build reproducible.


This change is Reviewable

@kl kl added the Android Issues related to Android label Jan 23, 2025
@kl kl requested review from faern, dlon and albin-mullvad January 23, 2025 12:59
This is needed for reproducible builds. This binary helper crate gets
the file paths to the user's Cargo and Rustup home dirs, as well as the
path to the Mullvad app source dir. These paths are then remapped to
fixed values which is needed to make the build reproducible.
@kl kl force-pushed the create-rustc-path-prefix-remap-crate branch from e11b401 to 43eeb65 Compare January 23, 2025 13:01
Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @kl)


Cargo.toml line 57 at r1 (raw file):

    "mullvad-daemon",
    "mullvad-version",
    "remap-path-prefix",

Please don't add a crate to the default members unless it's strictly needed. You can instead run the binary by adding -p remap-path-prefix to your invocation of cargo run.

We want to keep this list as short as possible both to bring down compile times during development as well as not maintaining two lists of crates.

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @kl)


remap-path-prefix/Cargo.toml line 3 at r1 (raw file):

[package]
name = "remap-path-prefix"
version = "0.1.0"

Please do not specify version of crates. Also make sure to inherit all other fields from the workspace. Including linting rules etc. Otherwise this crate risks deviating in code quality and other aspects. See other Cargo.toml files for what to include. You can basically just copy paste from them.

Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @kl)


remap-path-prefix/src/main.rs line 16 at r1 (raw file):

    let source_dir = env!("CARGO_MANIFEST_DIR")
        .split(concat!("/", env!("CARGO_PKG_NAME")))

I can't on the top of my head come up with a better solution, so maybe this is the best we can do. But this has two downsides:

  1. It's evaluated at compile time. Mean if the invocation of cargo run on this binary were to reuse a previous build built from another directory, this would point to the wrong source dir.
  2. It relies on the relative path to this crate relative to the workspace root. I checked, but unfortunately did not find any cargo env set that points to the workspace root dir. That would have been awesome.

Maybe we can mitigate issue 1 with a build.rs containing something like println!("cargo:rerun-if-env-changed=CARGO_MANIFEST_DIR");

Copy link

linear bot commented Jan 23, 2025

@mullvad mullvad deleted a comment from linear bot Jan 23, 2025
Copy link
Contributor Author

@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.

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @faern)


Cargo.toml line 57 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please don't add a crate to the default members unless it's strictly needed. You can instead run the binary by adding -p remap-path-prefix to your invocation of cargo run.

We want to keep this list as short as possible both to bring down compile times during development as well as not maintaining two lists of crates.

Done


remap-path-prefix/Cargo.toml line 3 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please do not specify version of crates. Also make sure to inherit all other fields from the workspace. Including linting rules etc. Otherwise this crate risks deviating in code quality and other aspects. See other Cargo.toml files for what to include. You can basically just copy paste from them.

Done

@kl
Copy link
Contributor Author

kl commented Jan 23, 2025

Superseded by #7512

@kl kl closed this Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants