don't install weak deps by default (#110)

* don't install weak deps

README.md

@@ -164,6 +164,10 @@ By default this field is enabled.
 
 *The /etc/os-release file can also be included by adding the distro's `<distro>-release` package to the packages array: this field exists to ensure the /etc/os-release file is included by default.*
 
+#### Weak dependencies
+
+rpmoci does not install [weak dependencies](https://docs.fedoraproject.org/en-US/packaging-guidelines/WeakDependencies/#:~:text=Weak%20dependencies%20should%20be%20used%20where%20possible%20to,require%20the%20full%20feature%20set%20of%20the%20package.), optimizing for small container image sizes.
+
 ### Image building
 
 Running `rpmoci build --image foo --tag bar` will build a container image in OCI format.

src/config.rs

@@ -101,7 +101,7 @@ pub(crate) enum Repository {
 #[derive(Debug, Serialize, Deserialize, Clone)]
 #[serde(deny_unknown_fields)]
 pub(crate) struct RepositoryDefinition {
-    id: Option<String>,
+    pub(crate) id: Option<String>,
     // The base url of the repository
     pub(crate) url: Url,
     /// Additional repository options.

src/lib.rs

@@ -65,7 +65,7 @@ pub fn main(command: Command) -> anyhow::Result<()> {
             let (cfg, lockfile_path, existing_lockfile) = load_config_and_lock_file(manifest_path)?;
 
             let lockfile = if let Ok(Some(lockfile)) = &existing_lockfile {
-                if lockfile.is_compatible(&cfg) && from_lockfile {
+                if lockfile.is_compatible_excluding_local_rpms(&cfg) && from_lockfile {
                     lockfile.resolve_from_previous(&cfg)?
                 } else {
                     if from_lockfile {
@@ -96,7 +96,9 @@ pub fn main(command: Command) -> anyhow::Result<()> {
             let (cfg, lockfile_path, existing_lockfile) = load_config_and_lock_file(manifest_path)?;
             let lockfile = match (existing_lockfile, locked) {
                 (Ok(Some(lockfile)), true) => {
-                    if !lockfile.is_compatible(&cfg) {
+                    // TODO: consider whether this can move to including local RPMs. (Subtlety here is that may
+                    // break scenarios where the user is using local RPMs that have a subset of the locked local RPM dependencies.)
+                    if !lockfile.is_compatible_excluding_local_rpms(&cfg) {
                         bail!(format!(
                             "the lock file {} needs to be updated but --locked was passed to prevent this",
                             lockfile_path.display()
@@ -105,7 +107,7 @@ pub fn main(command: Command) -> anyhow::Result<()> {
                     lockfile
                 }
                 (Ok(Some(lockfile)), false) => {
-                    if lockfile.all_local_deps_compatible(&cfg)? {
+                    if lockfile.is_compatible_including_local_rpms(&cfg)? {
                         // Compatible lockfile, use it
                         lockfile
                     } else {
@@ -184,7 +186,7 @@ pub fn main(command: Command) -> anyhow::Result<()> {
                 load_config_and_lock_file(manifest_path)?;
 
             if let Ok(Some(lockfile)) = existing_lockfile {
-                if lockfile.is_compatible(&cfg) {
+                if lockfile.is_compatible_excluding_local_rpms(&cfg) {
                     lockfile.download_rpms(&cfg, &out_dir)?;
                     lockfile.check_gpg_keys(&out_dir)?;
                 } else {

src/lockfile/mod.rs

@@ -33,11 +33,11 @@ mod resolve;
 pub struct Lockfile {
     pkg_specs: Vec<String>,
     packages: BTreeSet<Package>,
-    #[serde(default)]
+    #[serde(default, skip_serializing_if = "BTreeSet::is_empty")]
     local_packages: BTreeSet<LocalPackage>,
-    #[serde(default)]
+    #[serde(default, skip_serializing_if = "HashMap::is_empty")]
     repo_gpg_config: HashMap<String, RepoKeyInfo>,
-    #[serde(default)]
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
     global_key_specs: Vec<url::Url>,
 }
 
@@ -117,23 +117,28 @@ pub enum Algorithm {
 impl Lockfile {
     /// Returns true if the lockfile is compatible with the
     /// given configuration, false otherwise
+    ///
+    /// Local RPMs are not considered in this check, so this check can be performed
+    /// without them being present
     #[must_use]
-    pub fn is_compatible(&self, cfg: &Config) -> bool {
+    pub fn is_compatible_excluding_local_rpms(&self, cfg: &Config) -> bool {
         self.pkg_specs == cfg.contents.packages && self.global_key_specs == cfg.contents.gpgkeys
     }
 
-    /// Returns true if the lockfile is compatible with the specified configuration
-    /// and if all rpm packages required by local dependencies are included in the lockfile.
-    pub fn all_local_deps_compatible(&self, cfg: &Config) -> Result<bool> {
+    /// Returns true if the lockfile is compatible with the
+    /// given configuration, false otherwise
+    ///
+    /// This check also verifies that dependencies of local RPMs match those in the lockfile,
+    /// so requires the local RPMs to be present
+    pub fn is_compatible_including_local_rpms(&self, cfg: &Config) -> Result<bool> {
         let local_package_deps: BTreeSet<String> = self
             .local_packages
             .clone()
             .into_iter()
             .flat_map(|p| p.requires)
             .collect();
 
-        Ok(self.is_compatible(cfg)
-            // Verify dependencies of all local packages
+        Ok(self.is_compatible_excluding_local_rpms(cfg)
             && Self::read_local_rpm_deps(cfg)? == local_package_deps)
     }
 

src/lockfile/resolve.py

@@ -1,4 +1,5 @@
 """A dependency resolver for rpmoci"""
+
 # Copyright (C) Microsoft Corporation.
 #
 # This program is free software: you can redistribute it and/or modify
@@ -32,7 +33,7 @@ def resolve(base, packages):
     for pkg in pkgs:
         goal.install(pkg)
 
-    if not goal.run():
+    if not goal.run(ignore_weak_deps=True):
         msg = dnf.util._format_resolve_problems(goal.problem_rules())
         raise dnf.exceptions.DepsolveError(msg)
 

src/lockfile/resolve.rs

@@ -360,3 +360,40 @@ fn repo_password(repo_id: &str) -> Option<String> {
     ))
     .ok()
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{collections::HashMap, str::FromStr};
+
+    use url::Url;
+
+    use crate::{
+        config::{Repository, RepositoryDefinition},
+        lockfile::Lockfile,
+    };
+
+    #[test]
+    fn test_weak_deps() {
+        // prce2-tools in mariner recommends pcre2-docs. use this to test weak dep behaviour
+        let mut options = HashMap::new();
+        options.insert("gpgcheck".to_string(), "True".to_string());
+        options.insert("gpgkey".to_string(), "https://raw.githubusercontent.com/microsoft/CBL-Mariner/2.0/SPECS/mariner-repos/MICROSOFT-RPM-GPG-KEY,https://packages.microsoft.com/keys/microsoft.asc".to_string());
+
+        let mariner_repository = Repository::Definition(RepositoryDefinition {
+            id: Some("marinertest".to_string()),
+            url: Url::from_str("https://packages.microsoft.com/cbl-mariner/2.0/prod/base/x86_64")
+                .unwrap(),
+            options,
+        });
+        let repositories = vec![mariner_repository];
+
+        let lock = Lockfile::resolve(
+            vec!["pcre2-tools".to_string()],
+            &repositories,
+            Vec::new(),
+            true,
+        )
+        .unwrap();
+        assert!(!lock.packages.iter().any(|p| p.name == "pcre2-doc"));
+    }
+}

tests/fixtures/weakdeps/rpmoci.toml

@@ -0,0 +1,10 @@
+[contents]
+repositories = [
+    "https://packages.microsoft.com/cbl-mariner/2.0/prod/base/x86_64",
+]
+gpgkeys = [
+    "https://raw.githubusercontent.com/microsoft/CBL-Mariner/2.0/SPECS/mariner-repos/MICROSOFT-RPM-GPG-KEY",
+    "https://packages.microsoft.com/keys/microsoft.asc",
+]
+# This has a weak dependency on pcre2-docs 
+packages = ["pcre2-tools"]

tests/it.rs

@@ -318,5 +318,18 @@ fn test_explicit_etc_os_release() {
         .unwrap();
     let stderr = std::str::from_utf8(&output.stderr).unwrap();
     eprintln!("stderr: {}. {}. {}", stderr, root.display(), EXE);
+}
+
+#[test]
+fn test_weak_deps() {
+    // Verify a build without weak dependencies succeeds
+    let root = setup_test("weakdeps");
+    let output = Command::new(EXE)
+        .arg("build")
+        .arg("--image=weak")
+        .arg("--tag=deps")
+        .current_dir(&root)
+        .output()
+        .unwrap();
     assert!(output.status.success());
 }