Write lockfile external only (#3699)

## Description

Another attempt to improve
#3522

Current status/plan on the issue above was

- pre-commit hook that converts the `Cargo.lock` file into a
`Cargo.bazel.lock` file by removing internal deps
- use `crate.from_cargo` with the `Cargo.bazel.lock` file and
`skip_cargo_lockfile_overwrite = True`
- commit `MODULE.bazel.lock` which should have less merge conflicts
since the sha of `Cargo.bazel.lock` does not change that often

This works, but we need to make sure `Cargo.lock` is up to date. This
requires another step, so we replace a slow automated step (cargo
splicing) with an extra manual/non-bazel step (updating the `Cargo.lock`
file).

New idea to iterate

- use `crate.from_cargo` with the `Cargo.bazel.lock` file but instead of
using `skip_cargo_lockfile_overwrite = True`
- we let rules_rust do the cargo dependency resolution that is happening
as part of splicing
- instead of writing back the full content into the cargo lock file, we
only write back the third party (making the pre-commit hook obsolete).
For this, I introduced the
`strip_internal_dependencies_from_cargo_lockfile` flag

This would save us the pre-commit hook as well as the manual step to
update the `Cargo.lock` file. I already did a quick verification and
`remove_internal_dependencies_from_cargo_lockfile` achieves the same
results as the python implementation I listed in
#3522

## Open Points

This is an early draft to collect feedback. Once we aligned how exactly
we want to proceed, I would like to

- [ ] add/extend an integration test to have this tested end to end
- [ ] add and example to demonstrate how this should be used

---------

Co-authored-by: Daniel Wagner-Hall <[email protected]>

Author: hofbi

crate_universe/extensions.bzl

@@ -559,6 +559,7 @@ def _generate_hub_and_spokes(
         splicing_config,
         lockfile,
         skip_cargo_lockfile_overwrite,
+        strip_internal_dependencies_from_cargo_lockfile,
         cargo_lockfile = None,
         manifests = {},
         packages = {}):
@@ -575,6 +576,11 @@ def _generate_hub_and_spokes(
         skip_cargo_lockfile_overwrite (bool): Whether to skip writing the cargo lockfile back after resolving.
             You may want to set this if your dependency versions are maintained externally through a non-trivial set-up.
             But you probably don't want to set this.
+        strip_internal_dependencies_from_cargo_lockfile (bool): Whether to strip internal dependencies from the cargo lockfile.
+            You may want to use this if you want to maintain a cargo lockfile for bazel only.
+            Bazel only requires external dependencies to be present in the lockfile.
+            By removing internal dependencies, the lockfile changes less frequently which reduces merge conflicts
+            in other lockfiles where the cargo lockfile's sha is stored.
         cargo_lockfile (path): Path to Cargo.lock, if we have one.
         manifests (dict): The set of Cargo.toml manifests that apply to this closure, if any, keyed by path.
         packages (dict): The set of extra cargo crate tags that apply to this closure, if any, keyed by package name.
@@ -684,6 +690,7 @@ def _generate_hub_and_spokes(
         paths_to_track_file = paths_to_track_file,
         warnings_output_file = warnings_output_file,
         skip_cargo_lockfile_overwrite = skip_cargo_lockfile_overwrite,
+        strip_internal_dependencies_from_cargo_lockfile = strip_internal_dependencies_from_cargo_lockfile,
         **kwargs
     )
 
@@ -1169,6 +1176,7 @@ def _crate_impl(module_ctx):
                 manifests = manifests,
                 packages = packages,
                 skip_cargo_lockfile_overwrite = cfg.skip_cargo_lockfile_overwrite,
+                strip_internal_dependencies_from_cargo_lockfile = cfg.strip_internal_dependencies_from_cargo_lockfile,
             )
 
     metadata_kwargs = {}
@@ -1210,6 +1218,16 @@ _FROM_COMMON_ATTRS = {
         ),
         default = False,
     ),
+    "strip_internal_dependencies_from_cargo_lockfile": attr.bool(
+        doc = (
+            "Whether to strip internal dependencies from the cargo lockfile. " +
+            "You may want to use this if you want to maintain a cargo lockfile for bazel only. " +
+            "Bazel only requires external dependencies to be present in the lockfile. " +
+            "By removing internal dependencies, the lockfile changes less frequently which reduces merge conflicts " +
+            "in other lockfiles where the cargo lockfile's sha is stored."
+        ),
+        default = False,
+    ),
     "supported_platform_triples": attr.string_list(
         doc = "A set of all platform triples to consider when generating dependencies.",
         default = SUPPORTED_PLATFORM_TRIPLES,

crate_universe/private/crates_repository.bzl

@@ -122,6 +122,7 @@ def _crates_repository_impl(repository_ctx):
         paths_to_track_file = paths_to_track_file,
         warnings_output_file = warnings_output_file,
         skip_cargo_lockfile_overwrite = repository_ctx.attr.skip_cargo_lockfile_overwrite,
+        strip_internal_dependencies_from_cargo_lockfile = repository_ctx.attr.strip_internal_dependencies_from_cargo_lockfile,
         # sysroot = tools.sysroot,
         **kwargs
     )
@@ -377,6 +378,16 @@ CARGO_BAZEL_REPIN=1 CARGO_BAZEL_REPIN_ONLY=crate_index bazel sync --only=crate_i
                 "generate the value for this field. If unset, the defaults defined there will be used."
             ),
         ),
+        "strip_internal_dependencies_from_cargo_lockfile": attr.bool(
+            doc = (
+                "Whether to strip internal dependencies from the cargo lockfile. " +
+                "You may want to use this if you want to maintain a cargo lockfile for bazel only. " +
+                "Bazel only requires external dependencies to be present in the lockfile. " +
+                "By removing internal dependencies, the lockfile changes less frequently which reduces merge conflicts " +
+                "in other lockfiles where the cargo lockfile's sha is stored."
+            ),
+            default = False,
+        ),
         "supported_platform_triples": attr.string_list(
             doc = "A set of all platform triples to consider when generating dependencies.",
             default = SUPPORTED_PLATFORM_TRIPLES,

crate_universe/private/generate_utils.bzl

@@ -441,6 +441,7 @@ def execute_generator(
         paths_to_track_file,
         warnings_output_file,
         skip_cargo_lockfile_overwrite,
+        strip_internal_dependencies_from_cargo_lockfile,
         metadata = None,
         generator_label = None):
     """Execute the `cargo-bazel` binary to produce `BUILD` and `.bzl` files.
@@ -458,6 +459,11 @@ def execute_generator(
         skip_cargo_lockfile_overwrite (bool): Whether to skip writing the cargo lockfile back after resolving.
             You may want to set this if your dependency versions are maintained externally through a non-trivial set-up.
             But you probably don't want to set this.
+        strip_internal_dependencies_from_cargo_lockfile (bool): Whether to strip internal dependencies from the cargo lockfile.
+            You may want to use this if you want to maintain a cargo lockfile for bazel only.
+            Bazel only requires external dependencies to be present in the lockfile.
+            By removing internal dependencies, the lockfile changes less frequently which reduces merge conflicts
+            in other lockfiles where the cargo lockfile's sha is stored.
         generator_label (Label): The label of the `generator` parameter.
         metadata (path, optional): The path to a Cargo metadata json file. If this is set, it indicates to
             the generator that repinning is required. This file must be adjacent to a `Cargo.toml` and
@@ -499,6 +505,9 @@ def execute_generator(
     if skip_cargo_lockfile_overwrite:
         args.append("--skip-cargo-lockfile-overwrite")
 
+    if strip_internal_dependencies_from_cargo_lockfile:
+        args.append("--strip-internal-dependencies-from-cargo-lockfile")
+
     # Some components are not required unless re-pinning is enabled
     if metadata:
         args.extend([

crate_universe/src/cli/generate.rs

@@ -102,6 +102,14 @@ pub struct GenerateOptions {
     /// But you probably don't want to set this.
     #[clap(long)]
     pub skip_cargo_lockfile_overwrite: bool,
+
+    /// Whether to strip internal dependencies from the cargo lockfile.
+    /// You may want to use this if you want to maintain a cargo lockfile for bazel only.
+    /// Bazel only requires external dependencies to be present in the lockfile.
+    /// By removing internal dependencies, the lockfile changes less frequently which reduces merge conflicts
+    /// in other lockfiles where the cargo lockfile's sha is stored.
+    #[clap(long)]
+    pub strip_internal_dependencies_from_cargo_lockfile: bool,
 }
 
 pub fn generate(opt: GenerateOptions) -> Result<()> {
@@ -222,12 +230,31 @@ pub fn generate(opt: GenerateOptions) -> Result<()> {
     }
 
     if !opt.skip_cargo_lockfile_overwrite {
-        update_cargo_lockfile(&opt.cargo_lockfile, cargo_lockfile)?;
+        let cargo_lockfile_to_write = if opt.strip_internal_dependencies_from_cargo_lockfile {
+            remove_internal_dependencies_from_cargo_lockfile(cargo_lockfile)
+        } else {
+            cargo_lockfile
+        };
+        update_cargo_lockfile(&opt.cargo_lockfile, cargo_lockfile_to_write)?;
     }
 
     Ok(())
 }
 
+fn remove_internal_dependencies_from_cargo_lockfile(cargo_lockfile: Lockfile) -> Lockfile {
+    let filtered_packages: Vec<_> = cargo_lockfile
+        .packages
+        .into_iter()
+        // Filter packages to only keep external dependencies (those with a source)
+        .filter(|pkg| pkg.source.is_some())
+        .collect();
+
+    Lockfile {
+        packages: filtered_packages,
+        ..cargo_lockfile
+    }
+}
+
 fn update_cargo_lockfile(path: &Path, cargo_lockfile: Lockfile) -> Result<()> {
     let old_contents = fs::read_to_string(path).ok();
     let new_contents = cargo_lockfile.to_string();
@@ -293,3 +320,34 @@ fn write_paths_to_track<
     .context("Failed to write warnings file")?;
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::test;
+
+    #[test]
+    fn test_remove_internal_dependencies_from_cargo_lockfile_workspace_build_scripts_deps_should_remove_internal_dependencies(
+    ) {
+        let original_lockfile = test::lockfile::workspace_build_scripts_deps();
+
+        let filtered_lockfile =
+            remove_internal_dependencies_from_cargo_lockfile(original_lockfile.clone());
+
+        assert!(filtered_lockfile.packages.len() < original_lockfile.packages.len());
+
+        assert!(original_lockfile
+            .packages
+            .iter()
+            .any(|pkg| pkg.name.as_str() == "child"));
+        assert!(!filtered_lockfile
+            .packages
+            .iter()
+            .any(|pkg| pkg.name.as_str() == "child"));
+
+        assert!(filtered_lockfile
+            .packages
+            .iter()
+            .any(|pkg| pkg.name.as_str() == "anyhow"));
+    }
+}

examples/crate_universe/MODULE.bazel

@@ -16,10 +16,11 @@ local_path_override(
 )
 
 bazel_dep(name = "platforms", version = "1.0.0")
-bazel_dep(name = "rules_cc", version = "0.2.4")
+bazel_dep(name = "rules_cc", version = "0.2.8")
 bazel_dep(name = "bazel_skylib", version = "1.8.2")
 
 bazel_dep(name = "bazel_ci_rules", version = "1.0.0", dev_dependency = True)
+bazel_dep(name = "bazel_lib", version = "3.0.0", dev_dependency = True)
 
 dev = use_extension("//:vendor_extensions.bzl", "dev", dev_dependency = True)
 use_repo(
@@ -345,6 +346,41 @@ use_repo(
     "crate_index_cargo_workspace",
 )
 
+###############################################################################
+# R E M O V E   I N T E R N A L   D E P S
+###############################################################################
+
+# https://bazelbuild.github.io/rules_rust/crate_universe_bzlmod.html
+# Stripping internal dependencies from the cargo lockfile
+crate_index_remove_internal_deps = use_extension("@rules_rust//crate_universe:extensions.bzl", "crate")
+crate_index_remove_internal_deps.from_cargo(
+    name = "crate_index_remove_internal_deps",
+    cargo_lockfile = "//remove_internal_deps:Cargo.bazel.lock",
+    manifests = [
+        "//remove_internal_deps:Cargo.toml",
+    ],
+    strip_internal_dependencies_from_cargo_lockfile = True,
+)
+use_repo(
+    crate_index_remove_internal_deps,
+    "crate_index_remove_internal_deps",
+)
+
+# https://bazelbuild.github.io/rules_rust/crate_universe_bzlmod.html
+# Not stripping internal dependencies from the cargo lockfile as baseline
+crate_index_no_strip_internal_deps = use_extension("@rules_rust//crate_universe:extensions.bzl", "crate")
+crate_index_no_strip_internal_deps.from_cargo(
+    name = "crate_index_no_strip_internal_deps",
+    cargo_lockfile = "//remove_internal_deps:Cargo.lock",
+    manifests = [
+        "//remove_internal_deps:Cargo.toml",
+    ],
+)
+use_repo(
+    crate_index_no_strip_internal_deps,
+    "crate_index_no_strip_internal_deps",
+)
+
 ###############################################################################
 # C A R G O   C O N D I T I O N A L   D E P S
 ###############################################################################