#183 Automatically strip foreign dependencies
Merged a month ago by zbyszek. Opened 6 months ago by zbyszek.
fedora-rust/ zbyszek/rust2rpm cfg-handling  into  main

file modified
+144 -40
@@ -20,7 +20,7 @@ 

  import requests

  import tqdm

  

- from . import Metadata, licensing, generator

+ from . import Metadata, cfg, licensing, generator

  

  DEFAULT_EDITOR = "vi"

  XDG_CACHE_HOME = os.getenv("XDG_CACHE_HOME", os.path.expanduser("~/.cache"))
@@ -36,6 +36,18 @@ 

           (?:AGPL|APACHE|BSD|GFDL|GNU|L?GPL|MIT|MPL|OFL)-.*[0-9].*

           """, re.VERBOSE)

  

+ # [target.'cfg(not(any(target_os="windows", target_os="macos")))'.dependencies]

+ # [target."cfg(windows)".dependencies.winapi]

+ # [target."cfg(target_arch = \"wasm32\")".dev-dependencies.wasm-bindgen-test]

+ 

+ TARGET_DEPENDENCY_LINE = re.compile(r'''

+          ^ \[ target\.(?P<cfg>(?P<quote>['"])cfg\(.*\)(?P=quote))

+              \.

+              (?P<type> dependencies|build-dependencies|dev-dependencies)

+              (?:\. (?P<feature>[a-zA-Z0-9_-]+) )?

+          \] \s* $

+          ''', re.VERBOSE)

+ 

  def sortify(func):

      """Return a sorted list from a generator"""

      def wrapper(*args, **kwargs):
@@ -198,35 +210,119 @@ 

          license_files = get_license_files(root_path)

          yield toml, doc_files, license_files

  

- def make_patch(args, toml, tmpfile=False):

-     """Spawns editor on toml and returns a unified diff after editor closes"""

-     if not args.patch:

-         return []

+ def filter_out_features_re(dropped_features):

+     # This is a bit simplistic. But it doesn't seem worth the trouble to write

+     # a grammar for this. Can be always done later. If we parse this using a

+     # grammar, we beget the question how to preserve formatting idiosyncrasies.

+     # Regexp replacement makes it trivial to minimize changes.

+     match_features = '|'.join(dropped_features)

+     match_suffix = f'(?:/[{cfg.IDENT_CHARS[1]}]+)?'

+ 

+     return re.compile(fr'''(?P<comma> ,)? \s* (?P<quote>['"])

+                            ({match_features}) {match_suffix}

+                            (?P=quote) \s* (?(comma) |,?) \s*

+                       ''', re.VERBOSE)

+ 

+ def drop_foreign_dependencies(lines):

+     dropped_lines = 0

+     dropped_features = set()

+     good_lines = []

+ 

+     value = True

+     for line in lines:

+         # print(f'{line=}')

+         # [target.'cfg(not(any(target_os="windows", target_os="macos")))'.dependencies]

+         if m := TARGET_DEPENDENCY_LINE.match(line):

+             expr = m.group('cfg')

+             expr = ast.literal_eval(expr)

+             # print(f'matched: {expr=}')

+             try:

+                 value = cfg.parse_and_evaluate(expr)

+             except (ValueError, cfg.ParseException):

+                 print(f'Could not evaluate {expr!r}, treating as true')

+                 value = True

+ 

+             if not value:

+                 feature = m.group('feature')

+                 print(f'Skipping section {line.rstrip()} ({feature=})')

+                 dropped_features.add(feature)

+ 

+         elif line.startswith('['):

+             # previous section ended, let's keep printing lines again

+             value = True

+ 

+         # print(f'→ {value}')

+ 

+         if value:

+             good_lines += [line]

+         else:

+             dropped_lines += 1

+ 

+     if not dropped_lines:

+         # nothing to do, let's bail out

+         return None

+ 

+     good_lines2 = []

+     in_features = False

+     filt = filter_out_features_re(dropped_features)

+     for line in good_lines:

+         if line.rstrip() == '[features]':

+             in_features = True

+         elif line.startswith('['):

+             in_features = False

+         elif in_features:

+             line = re.sub(filt, '', line)

+             if not line:

+                 continue

+ 

+         good_lines2 += [line]

+ 

+     return good_lines2

+ 

+ 

+ def make_diff(path, lines1, mtime1, lines2, mtime2):

+     relpath = "/".join(path.split("/")[-2:])

+     return list(difflib.unified_diff(lines1, lines2,

+                                      fromfile=path, tofile=path,

+                                      fromfiledate=mtime1, tofiledate=mtime2))

+ 

  

-     editor = detect_editor()

+ def make_patches(args, toml):

+     """Returns up to two patches (automatic and manual).

  

+     For the manual patch, an editor is spawned on toml and a diff is

+     made after the editor returns.

+     """

      mtime_before = file_mtime(toml)

      toml_before = open(toml).readlines()

  

-     # When we are editing a git checkout, we should not modify the real file.

-     # When we are editing an unpacked crate, we are free to edit anything.

-     # Let's keep the file name as close as possible to make editing easier.

-     if tmpfile:

-         tmpfile = tempfile.NamedTemporaryFile("w+t", dir=os.path.dirname(toml),

-                                               prefix="Cargo.", suffix=".toml")

-         tmpfile.writelines(toml_before)

-         tmpfile.flush()

-         fname = tmpfile.name

+     diff1 = diff2 = None

+ 

+     # We always do the automatic part before asking the user for more edits.

+     if toml_after := args.patch_foreign and drop_foreign_dependencies(toml_before):

+         diff1 = make_diff(toml,

+                           toml_before, mtime_before,

+                           toml_after, mtime_before)

      else:

-         fname = toml

-     subprocess.check_call([editor, fname])

-     mtime_after = file_mtime(toml)

-     toml_after = open(fname).readlines()

-     toml_relpath = "/".join(toml.split("/")[-2:])

-     diff = list(difflib.unified_diff(toml_before, toml_after,

-                                      fromfile=toml_relpath, tofile=toml_relpath,

-                                      fromfiledate=mtime_before, tofiledate=mtime_after))

-     return diff

+         toml_after = toml_before

+ 

+     if args.patch:

+         with tempfile.NamedTemporaryFile("w+t", dir=os.path.dirname(toml),

+                                          prefix="Cargo.", suffix=".toml") as tmpfile:

+             tmpfile.writelines(toml_after)

+             tmpfile.flush()

+ 

+             editor = detect_editor()

+             subprocess.check_call([editor, tmpfile.name])

+ 

+             mtime_after2 = file_mtime(tmpfile.name)

+             toml_after2 = open(tmpfile.name).readlines()

+ 

+             diff2 = make_diff(toml,

+                               toml_after, mtime_before,

+                               toml_after2, mtime_after2)

+ 

+     return diff1, diff2

  

  def _is_path(path):

      return "/" in path or path in {".", ".."}
@@ -300,12 +396,12 @@ 

                  raise ValueError("--store-crate can only be used for a crate")

  

              toml, crate, version, doc_files, license_files = local_toml(crate, version)

-             diff = make_patch(args, toml, tmpfile=True)

+             diffs = make_patches(args, toml)

              metadata = Metadata.from_file(toml)

              if len(metadata) > 1:

                  print(f"Warning: multiple metadata for {toml}")

              metadata = metadata[0]

-             return metadata.name, diff, metadata, doc_files, license_files

+             return metadata.name, diffs, metadata, doc_files, license_files

      else:

          cratef, crate, version = download(crate, version)

  
@@ -313,14 +409,14 @@ 

          if not license_files:

              print(f"Warning: no license files detected in {crate}")

  

-         diff = make_patch(args, toml)

+         diffs = make_patches(args, toml)

          metadata = Metadata.from_file(toml)

          if len(metadata) > 1:

              print(f"Warning: multiple metadata for {toml}, ignoring everything except the first")

          metadata = metadata[0]

      if args.store_crate:

          shutil.copy2(cratef, os.path.join(os.getcwd(), f"{metadata.name}-{version}.crate"))

-     return crate, diff, metadata, doc_files, license_files

+     return crate, diffs, metadata, doc_files, license_files

  

  

  def detect_rpmautospec(default_target, spec_file):
@@ -417,8 +513,11 @@ 

      parser.add_argument("-t", "--target", action="store",

                          choices=("plain", "fedora", "mageia", "opensuse"), default=default_target,

                          help="Distribution target")

+     parser.add_argument("--no-patch-foreign", action="store_false",

+                         default=True, dest="patch_foreign",

+                         help="Do not automatically drop foreign dependencies in Cargo.toml")

      parser.add_argument("-p", "--patch", action="store_true",

-                         help="Do initial patching of Cargo.toml")

+                         help="Do manual patching of Cargo.toml")

      parser.add_argument("-s", "--store-crate", action="store_true",

                          help="Store crate in current directory")

      parser.add_argument("-a", "--rpmautospec", action="store_true",
@@ -469,11 +568,13 @@ 

          if args.crate is None:

              parser.error("crate/path argument missing and autodetection failed")

  

-     crate, diff, metadata, doc_files, license_files = make_diff_metadata(args, args.crate, args.version)

- 

-     patch_file = f"{metadata.name}-fix-metadata.diff" if diff else None

+     crate, diffs, metadata, doc_files, license_files = make_diff_metadata(args, args.crate, args.version)

  

      pkg_name = package_name_suffixed(metadata.name, args.suffix)

+ 

+     patch_files = (f"{metadata.name}-automatic.diff" if diffs[0] else None,

+                    f"{metadata.name}-manual.diff" if diffs[1] else None)

+ 

      spec_file = pathlib.Path(f"{pkg_name}.spec")

  

      if args.target in {"fedora"} and args.existence_check and not os.path.exists(spec_file):
@@ -522,7 +623,8 @@ 

          pkg_name = pkg_name,

          crate = crate,

          metadata = metadata,

-         patch_file = patch_file,

+         patch_file_automatic=patch_files[0],

+         patch_file_manual=patch_files[1],

          packager_identity = packager_identity,

          doc_files = doc_files,

          license_files = license_files,
@@ -533,18 +635,20 @@ 

      if args.stdout:

          print(f"# {spec_file}")

          print(spec_contents)

-         if patch_file is not None:

-             print(f"# {patch_file}")

-             print("".join(diff), end="")

+         for fname, diff in zip(patch_files, diffs):

+             if fname:

+                 print(f"# {fname}")

+                 print("".join(diff), end="")

      else:

          with open(spec_file, "w") as fobj:

              fobj.write(spec_contents)

              fobj.write("\n")

          print(f'Wrote {fobj.name}')

-         if patch_file is not None:

-             with open(patch_file, "w") as fobj:

-                 fobj.writelines(diff)

-             print(f'Wrote {fobj.name}')

+         for fname, diff in zip(patch_files, diffs):

+             if fname:

+                 with open(fname, "w") as fobj:

+                     fobj.writelines(diff)

+                 print(f'Wrote {fobj.name}')

  

  if __name__ == "__main__":

      main()

file added
+120
@@ -0,0 +1,120 @@ 

+ import ast

+ import ctypes

+ import functools

+ import platform

+ import sys

+ 

+ import pyparsing as pp

+ from pyparsing.exceptions import ParseException

+ 

+ pp.ParserElement.enable_packrat()

+ 

+ # ConfigurationPredicate :

+ #      ConfigurationOption

+ #    | ConfigurationAll

+ #    | ConfigurationAny

+ #    | ConfigurationNot

+ # ConfigurationOption :

+ #    IDENTIFIER (= (STRING_LITERAL | RAW_STRING_LITERAL))?

+ # ConfigurationAll

+ #    all ( ConfigurationPredicateList? )

+ # ConfigurationAny

+ #    any ( ConfigurationPredicateList? )

+ # ConfigurationNot

+ #    not ( ConfigurationPredicate )

+ # ConfigurationPredicateList

+ #    ConfigurationPredicate (, ConfigurationPredicate)* ,?

+ 

+ # cfg(target_os = "macos")

+ # cfg(any(foo, bar))

+ # cfg(all(unix, target_pointer_width = "32"))

+ # cfg(not(foo))

+ 

+ IDENT_CHARS = pp.alphas + '_', pp.alphanums + '_'

+ 

+ def _call(word, arg):

+     return pp.Group(pp.Literal(word) + pp.Suppress('(') + arg + pp.Suppress(')'), aslist=True)

+ 

+ @functools.cache

+ def cfg_grammar():

+     pred = pp.Forward()

+ 

+     ident = pp.Word(IDENT_CHARS[0], IDENT_CHARS[1])

+     option = pp.Group(ident + pp.Optional(pp.Suppress('=') + pp.quotedString), aslist=True)

+ 

+     not_ = _call('not', pred)

+ 

+     # pp.pyparsing_common.comma_separated_list?

+     # any_ = _call('any', pp.pyparsing_common.comma_separated_list(pred))

+     # all_ = _call('all', pp.pyparsing_common.comma_separated_list(pred))

+     # all_ = _call('all', pp.delimited_list(pred))

+ 

+     any_ = _call('any', pred + pp.ZeroOrMore(pp.Suppress(',') + pred))

+     all_ = _call('all', pred + pp.ZeroOrMore(pp.Suppress(',') + pred))

+ 

+     pred <<= not_ | any_ | all_ | option

+ 

+     grammar = _call('cfg', pred)

+     return grammar

+ 

+ @functools.cache

+ def evaluate_variable(name):

+     # print(f'evaluate_variable: {expr}')

+     match name:

+         case 'target_arch':

+             return platform.machine()

+ 

+         case 'target_os':

+             return 'linux'

+ 

+         case 'target_family':

+             return 'unix'

+ 

+         case 'unix':

+             return evaluate_variable('target_family') == 'unix'

+ 

+         case 'windows':

+             return evaluate_variable('target_family') == 'windows'

+ 

+         case 'target_env':

+             # Key-value option set with further disambiguating information about the

+             # target platform with information about the ABI or libc used

+             return ...

+ 

+         case 'target_endian':

+             return sys.byteorder

+ 

+         case 'target_pointer_width':

+             return str(ctypes.sizeof(ctypes.c_voidp) * 8)

+ 

+         case 'target_vendor':

+             return 'unknown'

+ 

+         case _:

+             print(f'Unknown variable {name}, assuming False')

+             return False

+ 

+ def evaluate(expr, nested=False):

+     # print(f'evaluate: {expr}')

+     match expr:

+         case ['cfg', expr] if not nested:

+             return evaluate(expr, True)

+         case ['not', expr] if nested:

+             return not evaluate(expr, True)

+         case ['all', *args] if nested:

+             return all(evaluate(arg, True) for arg in args)

+         case ['any', *args] if nested:

+             return any(evaluate(arg, True) for arg in args)

+         case [variable, value] if nested:

+             v = ast.literal_eval(value)

+             x = evaluate_variable(variable)

+             return x == v

+         case [variable] if nested:

+             x = evaluate_variable(variable)

+             return x

+         case _:

+             raise ValueError

+ 

+ def parse_and_evaluate(expr):

+     parsed = cfg_grammar().parse_string(expr)

+     return evaluate(parsed[0])

file modified
+4 -2
@@ -29,7 +29,8 @@ 

          pkg_name,

          crate,

          metadata,

-         patch_file,

+         patch_file_manual,

+         patch_file_automatic,

          packager_identity,

          doc_files,

          license_files,
@@ -105,7 +106,8 @@ 

  

      spec_contents = template.render(

          metadata=metadata,

-         patch_file=patch_file,

+         patch_file_manual=patch_file_manual,

+         patch_file_automatic=patch_file_automatic,

          **kwargs) + '\n'

  

      return spec_contents

file modified
+8 -4
@@ -34,13 +34,17 @@ 

  {% endif %}

  URL:            https://crates.io/crates/{{ crate }}

  Source:         %{crates_source}

- {% if patch_file is not none %}

+ {% if patch_file_automatic is not none %}

+ # Automatically generated patch to strip foreign dependencies

+ Patch:          {{ patch_file_automatic }}

+ {% endif %}

+ {% if patch_file_manual is not none %}

  {% if target == "opensuse" %}

- # PATCH-FIX-OPENSUSE {{ patch_file }} -- Initial patched metadata

+ # PATCH-FIX-OPENSUSE {{ patch_file_manual }} — Maintainer patch to adjust dependencies

  {% else %}

- # Initial patched metadata

+ # Maintainer patch to adjust dependencies

  {% endif %}

- Patch:          {{ patch_file }}

+ Patch:          {{ patch_file_manual }}

  {% endif %}

  

  ExclusiveArch:  %{rust_arches}

@@ -12,8 +12,10 @@ 

  License:        MIT OR Apache-2.0

  URL:            https://crates.io/crates/cxx-build-1.0.71

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          cxx-build-1.0.71-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          cxx-build-1.0.71-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  

@@ -14,8 +14,10 @@ 

  License:        MIT or ASL 2.0

  URL:            https://crates.io/crates/cxx-build-1.0.71

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          cxx-build-1.0.71-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          cxx-build-1.0.71-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  

@@ -30,8 +30,10 @@ 

  License:        MIT OR Apache-2.0

  URL:            https://crates.io/crates/cxx-build-1.0.71

  Source:         %{crates_source}

- # PATCH-FIX-OPENSUSE cxx-build-1.0.71-patch1.diff -- Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          cxx-build-1.0.71-patch1.diff

+ # PATCH-FIX-OPENSUSE cxx-build-1.0.71-patch2.diff — Maintainer patch to adjust dependencies

+ Patch:          cxx-build-1.0.71-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  

@@ -12,8 +12,10 @@ 

  License:        MIT OR Apache-2.0

  URL:            https://crates.io/crates/cxx-build-1.0.71

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          cxx-build-1.0.71-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          cxx-build-1.0.71-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  

@@ -12,8 +12,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/nix-0.24.1

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          nix-0.24.1-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          nix-0.24.1-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -30,7 +32,6 @@ 

  BuildRequires:  (crate(parking_lot/default) >= 0.11.2 with crate(parking_lot/default) < 0.12.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(semver/default) >= 1.0.0 with crate(semver/default) < 2.0.0~)

- BuildRequires:  (crate(sysctl/default) >= 0.1.0 with crate(sysctl/default) < 0.2.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.2.0 with crate(tempfile/default) < 4.0.0~)

  %endif

  

@@ -13,8 +13,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/nix-0.24.1

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          nix-0.24.1-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          nix-0.24.1-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -31,7 +33,6 @@ 

  BuildRequires:  (crate(parking_lot/default) >= 0.11.2 with crate(parking_lot/default) < 0.12.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(semver/default) >= 1.0.0 with crate(semver/default) < 2.0.0~)

- BuildRequires:  (crate(sysctl/default) >= 0.1.0 with crate(sysctl/default) < 0.2.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.2.0 with crate(tempfile/default) < 4.0.0~)

  %endif

  

@@ -30,8 +30,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/nix-0.24.1

  Source:         %{crates_source}

- # PATCH-FIX-OPENSUSE nix-0.24.1-patch1.diff -- Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          nix-0.24.1-patch1.diff

+ # PATCH-FIX-OPENSUSE nix-0.24.1-patch2.diff — Maintainer patch to adjust dependencies

+ Patch:          nix-0.24.1-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -48,7 +50,6 @@ 

  BuildRequires:  (crate(parking_lot/default) >= 0.11.2 with crate(parking_lot/default) < 0.12.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(semver/default) >= 1.0.0 with crate(semver/default) < 2.0.0~)

- BuildRequires:  (crate(sysctl/default) >= 0.1.0 with crate(sysctl/default) < 0.2.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.2.0 with crate(tempfile/default) < 4.0.0~)

  %endif

  

@@ -12,8 +12,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/nix-0.24.1

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          nix-0.24.1-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          nix-0.24.1-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -30,7 +32,6 @@ 

  BuildRequires:  (crate(parking_lot/default) >= 0.11.2 with crate(parking_lot/default) < 0.12.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(semver/default) >= 1.0.0 with crate(semver/default) < 2.0.0~)

- BuildRequires:  (crate(sysctl/default) >= 0.1.0 with crate(sysctl/default) < 0.2.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.2.0 with crate(tempfile/default) < 4.0.0~)

  %endif

  

@@ -12,8 +12,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/tokio-1.19.2

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          tokio-1.19.2-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          tokio-1.19.2-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -24,22 +26,15 @@ 

  BuildRequires:  (crate(futures/async-await) >= 0.3.0 with crate(futures/async-await) < 0.4.0~)

  BuildRequires:  (crate(futures/default) >= 0.3.0 with crate(futures/default) < 0.4.0~)

  BuildRequires:  (crate(libc/default) >= 0.2.42 with crate(libc/default) < 0.3.0~)

- BuildRequires:  (crate(loom/checkpoint) >= 0.5.2 with crate(loom/checkpoint) < 0.6.0~)

- BuildRequires:  (crate(loom/default) >= 0.5.2 with crate(loom/default) < 0.6.0~)

- BuildRequires:  (crate(loom/futures) >= 0.5.2 with crate(loom/futures) < 0.6.0~)

- BuildRequires:  (crate(mio-aio/default) >= 0.6.0 with crate(mio-aio/default) < 0.7.0~)

- BuildRequires:  (crate(mio-aio/tokio) >= 0.6.0 with crate(mio-aio/tokio) < 0.7.0~)

  BuildRequires:  (crate(mockall/default) >= 0.11.1 with crate(mockall/default) < 0.12.0~)

  BuildRequires:  (crate(nix/fs) >= 0.24.0 with crate(nix/fs) < 0.25.0~)

  BuildRequires:  (crate(nix/socket) >= 0.24.0 with crate(nix/socket) < 0.25.0~)

- BuildRequires:  (crate(ntapi/default) >= 0.3.6 with crate(ntapi/default) < 0.4.0~)

  BuildRequires:  (crate(proptest/default) >= 1.0.0 with crate(proptest/default) < 2.0.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(socket2/default) >= 0.4.0 with crate(socket2/default) < 0.5.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.1.0 with crate(tempfile/default) < 4.0.0~)

  BuildRequires:  (crate(tokio-stream/default) >= 0.1.0 with crate(tokio-stream/default) < 0.2.0~)

  BuildRequires:  (crate(tokio-test/default) >= 0.4.0 with crate(tokio-test/default) < 0.5.0~)

- BuildRequires:  (crate(wasm-bindgen-test/default) >= 0.3.0 with crate(wasm-bindgen-test/default) < 0.4.0~)

  %endif

  

  %global _description %{expand:
@@ -376,30 +371,6 @@ 

  %files       -n %{name}+tokio-macros-devel

  %ghost %{crate_instdir}/Cargo.toml

  

- %package     -n %{name}+tracing-devel

- Summary:        %{summary}

- BuildArch:      noarch

- 

- %description -n %{name}+tracing-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "tracing" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+tracing-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

- %package     -n %{name}+winapi-devel

- Summary:        %{summary}

- BuildArch:      noarch

- 

- %description -n %{name}+winapi-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "winapi" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+winapi-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

  %prep

  %autosetup -n %{real_crate}-%{version_no_tilde} -p1

  %cargo_prep

@@ -13,8 +13,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/tokio-1.19.2

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          tokio-1.19.2-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          tokio-1.19.2-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -25,22 +27,15 @@ 

  BuildRequires:  (crate(futures/async-await) >= 0.3.0 with crate(futures/async-await) < 0.4.0~)

  BuildRequires:  (crate(futures/default) >= 0.3.0 with crate(futures/default) < 0.4.0~)

  BuildRequires:  (crate(libc/default) >= 0.2.42 with crate(libc/default) < 0.3.0~)

- BuildRequires:  (crate(loom/checkpoint) >= 0.5.2 with crate(loom/checkpoint) < 0.6.0~)

- BuildRequires:  (crate(loom/default) >= 0.5.2 with crate(loom/default) < 0.6.0~)

- BuildRequires:  (crate(loom/futures) >= 0.5.2 with crate(loom/futures) < 0.6.0~)

- BuildRequires:  (crate(mio-aio/default) >= 0.6.0 with crate(mio-aio/default) < 0.7.0~)

- BuildRequires:  (crate(mio-aio/tokio) >= 0.6.0 with crate(mio-aio/tokio) < 0.7.0~)

  BuildRequires:  (crate(mockall/default) >= 0.11.1 with crate(mockall/default) < 0.12.0~)

  BuildRequires:  (crate(nix/fs) >= 0.24.0 with crate(nix/fs) < 0.25.0~)

  BuildRequires:  (crate(nix/socket) >= 0.24.0 with crate(nix/socket) < 0.25.0~)

- BuildRequires:  (crate(ntapi/default) >= 0.3.6 with crate(ntapi/default) < 0.4.0~)

  BuildRequires:  (crate(proptest/default) >= 1.0.0 with crate(proptest/default) < 2.0.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(socket2/default) >= 0.4.0 with crate(socket2/default) < 0.5.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.1.0 with crate(tempfile/default) < 4.0.0~)

  BuildRequires:  (crate(tokio-stream/default) >= 0.1.0 with crate(tokio-stream/default) < 0.2.0~)

  BuildRequires:  (crate(tokio-test/default) >= 0.4.0 with crate(tokio-test/default) < 0.5.0~)

- BuildRequires:  (crate(wasm-bindgen-test/default) >= 0.3.0 with crate(wasm-bindgen-test/default) < 0.4.0~)

  %endif

  

  %global _description %{expand:
@@ -404,32 +399,6 @@ 

  %files       -n %{name}+tokio-macros-devel

  %ghost %{crate_instdir}/Cargo.toml

  

- %package     -n %{name}+tracing-devel

- Summary:        %{summary}

- Group:          Development/Rust

- BuildArch:      noarch

- 

- %description -n %{name}+tracing-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "tracing" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+tracing-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

- %package     -n %{name}+winapi-devel

- Summary:        %{summary}

- Group:          Development/Rust

- BuildArch:      noarch

- 

- %description -n %{name}+winapi-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "winapi" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+winapi-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

  %prep

  %autosetup -n %{real_crate}-%{version_no_tilde} -p1

  %cargo_prep

@@ -30,8 +30,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/tokio-1.19.2

  Source:         %{crates_source}

- # PATCH-FIX-OPENSUSE tokio-1.19.2-patch1.diff -- Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          tokio-1.19.2-patch1.diff

+ # PATCH-FIX-OPENSUSE tokio-1.19.2-patch2.diff — Maintainer patch to adjust dependencies

+ Patch:          tokio-1.19.2-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -42,22 +44,15 @@ 

  BuildRequires:  (crate(futures/async-await) >= 0.3.0 with crate(futures/async-await) < 0.4.0~)

  BuildRequires:  (crate(futures/default) >= 0.3.0 with crate(futures/default) < 0.4.0~)

  BuildRequires:  (crate(libc/default) >= 0.2.42 with crate(libc/default) < 0.3.0~)

- BuildRequires:  (crate(loom/checkpoint) >= 0.5.2 with crate(loom/checkpoint) < 0.6.0~)

- BuildRequires:  (crate(loom/default) >= 0.5.2 with crate(loom/default) < 0.6.0~)

- BuildRequires:  (crate(loom/futures) >= 0.5.2 with crate(loom/futures) < 0.6.0~)

- BuildRequires:  (crate(mio-aio/default) >= 0.6.0 with crate(mio-aio/default) < 0.7.0~)

- BuildRequires:  (crate(mio-aio/tokio) >= 0.6.0 with crate(mio-aio/tokio) < 0.7.0~)

  BuildRequires:  (crate(mockall/default) >= 0.11.1 with crate(mockall/default) < 0.12.0~)

  BuildRequires:  (crate(nix/fs) >= 0.24.0 with crate(nix/fs) < 0.25.0~)

  BuildRequires:  (crate(nix/socket) >= 0.24.0 with crate(nix/socket) < 0.25.0~)

- BuildRequires:  (crate(ntapi/default) >= 0.3.6 with crate(ntapi/default) < 0.4.0~)

  BuildRequires:  (crate(proptest/default) >= 1.0.0 with crate(proptest/default) < 2.0.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(socket2/default) >= 0.4.0 with crate(socket2/default) < 0.5.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.1.0 with crate(tempfile/default) < 4.0.0~)

  BuildRequires:  (crate(tokio-stream/default) >= 0.1.0 with crate(tokio-stream/default) < 0.2.0~)

  BuildRequires:  (crate(tokio-test/default) >= 0.4.0 with crate(tokio-test/default) < 0.5.0~)

- BuildRequires:  (crate(wasm-bindgen-test/default) >= 0.3.0 with crate(wasm-bindgen-test/default) < 0.4.0~)

  %endif

  

  %global _description %{expand:
@@ -421,32 +416,6 @@ 

  %files       -n %{name}+tokio-macros-devel

  %ghost %{crate_instdir}/Cargo.toml

  

- %package     -n %{name}+tracing-devel

- Summary:        %{summary}

- Group:          Development/Libraries/Rust

- BuildArch:      noarch

- 

- %description -n %{name}+tracing-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "tracing" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+tracing-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

- %package     -n %{name}+winapi-devel

- Summary:        %{summary}

- Group:          Development/Libraries/Rust

- BuildArch:      noarch

- 

- %description -n %{name}+winapi-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "winapi" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+winapi-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

  %prep

  %autosetup -n %{real_crate}-%{version_no_tilde} -p1

  %cargo_prep

@@ -12,8 +12,10 @@ 

  License:        MIT

  URL:            https://crates.io/crates/tokio-1.19.2

  Source:         %{crates_source}

- # Initial patched metadata

+ # Automatically generated patch to strip foreign dependencies

  Patch:          tokio-1.19.2-patch1.diff

+ # Maintainer patch to adjust dependencies

+ Patch:          tokio-1.19.2-patch2.diff

  

  ExclusiveArch:  %{rust_arches}

  
@@ -24,22 +26,15 @@ 

  BuildRequires:  (crate(futures/async-await) >= 0.3.0 with crate(futures/async-await) < 0.4.0~)

  BuildRequires:  (crate(futures/default) >= 0.3.0 with crate(futures/default) < 0.4.0~)

  BuildRequires:  (crate(libc/default) >= 0.2.42 with crate(libc/default) < 0.3.0~)

- BuildRequires:  (crate(loom/checkpoint) >= 0.5.2 with crate(loom/checkpoint) < 0.6.0~)

- BuildRequires:  (crate(loom/default) >= 0.5.2 with crate(loom/default) < 0.6.0~)

- BuildRequires:  (crate(loom/futures) >= 0.5.2 with crate(loom/futures) < 0.6.0~)

- BuildRequires:  (crate(mio-aio/default) >= 0.6.0 with crate(mio-aio/default) < 0.7.0~)

- BuildRequires:  (crate(mio-aio/tokio) >= 0.6.0 with crate(mio-aio/tokio) < 0.7.0~)

  BuildRequires:  (crate(mockall/default) >= 0.11.1 with crate(mockall/default) < 0.12.0~)

  BuildRequires:  (crate(nix/fs) >= 0.24.0 with crate(nix/fs) < 0.25.0~)

  BuildRequires:  (crate(nix/socket) >= 0.24.0 with crate(nix/socket) < 0.25.0~)

- BuildRequires:  (crate(ntapi/default) >= 0.3.6 with crate(ntapi/default) < 0.4.0~)

  BuildRequires:  (crate(proptest/default) >= 1.0.0 with crate(proptest/default) < 2.0.0~)

  BuildRequires:  (crate(rand/default) >= 0.8.0 with crate(rand/default) < 0.9.0~)

  BuildRequires:  (crate(socket2/default) >= 0.4.0 with crate(socket2/default) < 0.5.0~)

  BuildRequires:  (crate(tempfile/default) >= 3.1.0 with crate(tempfile/default) < 4.0.0~)

  BuildRequires:  (crate(tokio-stream/default) >= 0.1.0 with crate(tokio-stream/default) < 0.2.0~)

  BuildRequires:  (crate(tokio-test/default) >= 0.4.0 with crate(tokio-test/default) < 0.5.0~)

- BuildRequires:  (crate(wasm-bindgen-test/default) >= 0.3.0 with crate(wasm-bindgen-test/default) < 0.4.0~)

  %endif

  

  %global _description %{expand:
@@ -256,7 +251,6 @@ 

  Requires:       (crate(mio/net) >= 0.8.1 with crate(mio/net) < 0.9.0~)

  Requires:       (crate(mio/os-ext) >= 0.8.1 with crate(mio/os-ext) < 0.9.0~)

  Requires:       (crate(mio/os-poll) >= 0.8.1 with crate(mio/os-poll) < 0.9.0~)

- Requires:       (crate(winapi/namedpipeapi) >= 0.3.8 with crate(winapi/namedpipeapi) < 0.4.0~)

  Requires:       crate(tokio) = 1.19.2

  Requires:       crate(tokio/libc) = 1.19.2

  Requires:       crate(tokio/socket2) = 1.19.2
@@ -325,7 +319,6 @@ 

  Requires:       (crate(mio/net) >= 0.8.1 with crate(mio/net) < 0.9.0~)

  Requires:       (crate(mio/os-ext) >= 0.8.1 with crate(mio/os-ext) < 0.9.0~)

  Requires:       (crate(mio/os-poll) >= 0.8.1 with crate(mio/os-poll) < 0.9.0~)

- Requires:       (crate(winapi/threadpoollegacyapiset) >= 0.3.8 with crate(winapi/threadpoollegacyapiset) < 0.4.0~)

  Requires:       crate(tokio) = 1.19.2

  Requires:       crate(tokio/bytes) = 1.19.2

  Requires:       crate(tokio/libc) = 1.19.2
@@ -381,7 +374,6 @@ 

  Requires:       (crate(mio/net) >= 0.8.1 with crate(mio/net) < 0.9.0~)

  Requires:       (crate(mio/os-ext) >= 0.8.1 with crate(mio/os-ext) < 0.9.0~)

  Requires:       (crate(mio/os-poll) >= 0.8.1 with crate(mio/os-poll) < 0.9.0~)

- Requires:       (crate(winapi/consoleapi) >= 0.3.8 with crate(winapi/consoleapi) < 0.4.0~)

  Requires:       crate(tokio) = 1.19.2

  Requires:       crate(tokio/libc) = 1.19.2

  Requires:       crate(tokio/once_cell) = 1.19.2
@@ -507,43 +499,6 @@ 

  %files       -n %{name}+tokio-macros-devel

  %ghost %{crate_instdir}/Cargo.toml

  

- %package     -n %{name}+tracing-devel

- Summary:        %{summary}

- BuildArch:      noarch

- Provides:       crate(tokio/tracing) = 1.19.2

- Requires:       cargo

- Requires:       (crate(tracing/std) >= 0.1.25 with crate(tracing/std) < 0.2.0~)

- Requires:       crate(tokio) = 1.19.2

- 

- %description -n %{name}+tracing-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "tracing" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+tracing-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

- %package     -n %{name}+winapi-devel

- Summary:        %{summary}

- BuildArch:      noarch

- Provides:       crate(tokio/winapi) = 1.19.2

- Requires:       cargo

- Requires:       (crate(winapi/handleapi) >= 0.3.8 with crate(winapi/handleapi) < 0.4.0~)

- Requires:       (crate(winapi/mswsock) >= 0.3.8 with crate(winapi/mswsock) < 0.4.0~)

- Requires:       (crate(winapi/std) >= 0.3.8 with crate(winapi/std) < 0.4.0~)

- Requires:       (crate(winapi/winsock2) >= 0.3.8 with crate(winapi/winsock2) < 0.4.0~)

- Requires:       (crate(winapi/ws2ipdef) >= 0.3.8 with crate(winapi/ws2ipdef) < 0.4.0~)

- Requires:       (crate(winapi/ws2tcpip) >= 0.3.8 with crate(winapi/ws2tcpip) < 0.4.0~)

- Requires:       crate(tokio) = 1.19.2

- 

- %description -n %{name}+winapi-devel %{_description}

- 

- This package contains library source intended for building other packages which

- use the "winapi" feature of the "%{crate}" crate.

- 

- %files       -n %{name}+winapi-devel

- %ghost %{crate_instdir}/Cargo.toml

- 

  %prep

  %autosetup -n %{real_crate}-%{version_no_tilde} -p1

  %cargo_prep

@@ -0,0 +1,59 @@ 

+ import pytest

+ 

+ from rust2rpm import cfg

+ 

+ def test_pyparsing_run_tests():

+     g = cfg.cfg_grammar()

+ 

+     g.run_tests("""\

+     cfg(target_os = "macos")

+     cfg(any(foo, bar))

+     cfg(all(unix, target_pointer_width = "32"))

+     cfg(not(foo))

+     """)

+ 

+ @pytest.mark.parametrize('expr, expected', [

+     ('cfg(target_os = "macos")',

+      False),

+     ('cfg(any(foo, bar))',

+      False),

+     ('cfg(all(unix, target_pointer_width = "16"))',

+      False),

+     ('cfg(not(foo))',

+      True),

+ 

+     ('cfg(unix)',

+      True),

+     ('cfg(not(unix))',

+      False),

+     ('cfg(windows)',

+      False),

+     ('cfg(linux)',   # not defined

+      False),

+     ('cfg(not(windows))',

+      True),

+     ('cfg(any(unix, windows))',

+      True),

+     ('cfg(any(windows, unix))',

+      True),

+     ('cfg(any(windows, windows, windows))',

+      False),

+ 

+     ('cfg(target_os = "linux")',

+      True),

+     ('cfg(any(target_os = "linux"))',

+      True),

+     ('cfg(all(target_os = "linux"))',

+      True),

+     ('cfg(any(target_os = "linux", target_os = "macos"))',

+      True),

+ 

+     ('cfg(any(target_pointer_width = "16", target_pointer_width = "32", target_pointer_width = "64"))',

+      True),

+     ('cfg(all(target_pointer_width = "16", target_pointer_width = "32", target_pointer_width = "64"))',

+      False),

+ 

+ ])

+ def test_expressions(expr, expected):

You could take a look at the test cases I wrote for the cfg expression evaluator I wrote a while ago:
https://github.com/ironthree/cargoman/blob/master/src/eval.rs#L78

The test cases were "inspired" by cfg expressions that I actually came across when dealing with various packages.

+     value = cfg.parse_and_evaluate(expr)

+     assert value == expected

@@ -10,7 +10,8 @@ 

  from rust2rpm.__main__ import (

      Metadata,

      get_parser,

-     package_name_suffixed)

+     package_name_suffixed,

+     drop_foreign_dependencies)

  

  

  def test_to_list():
@@ -47,9 +48,10 @@ 

      pkg_name = package_name_suffixed(crate, args.suffix)

  

      toml_before = open(tomlfile).readlines()

+     toml_after = drop_foreign_dependencies(toml_before) or toml_before

  

      fake_toml = tmpdir / 'Cargo.toml'

-     fake_toml.write_text('\n'.join(toml_before))

+     fake_toml.write_text('\n'.join(toml_after))

  

      metadata, = Metadata.from_file(fake_toml)

  
@@ -58,7 +60,8 @@ 

          pkg_name = pkg_name,

          crate = crate,

          metadata = metadata,

-         patch_file = f'{crate}-patch1.diff',

+         patch_file_automatic = f'{crate}-patch1.diff',

+         patch_file_manual = f'{crate}-patch2.diff',

          packager_identity = 'Jane Jane <jane@jane.org>',

          doc_files = ['DOC1', 'DOC2'],

          license_files = ['LIC1', 'LIC2'],

The basic workflow is that very similarly to how we would open
an editor to do manual patching of Cargo.toml, we automatically
rewrite the file first. In this automatic rewrite, for any sections like
[target.cfg(…)], we evaluate the conditional and drop the whole section
if the conditional is known to be false.

After the automatic filtering the editor is opened for the user to do
further manual patching, if -p was used.

The automatic and manual changes together land in the patch.

When the conditional cannot be evaluated because it uses some form that
we don't understand, it is left untouched. But it seems that the
conditionals used in practice don't have too much variety. If parser
and evaluator certainly don't cover all the corner cases, but it might
not matter in practice. We can always fix them later if unsupported cases
are found.

Fixes https://pagure.io/fedora-rust/rust2rpm/issue/2.

TODO: I think we should keep the patches for automatic and manual patching
separate. This would make it easier to reapply manual changes after an
upgrade.

Note that simply stripping those dependencies might lead to problems in some circumstances, for example, if those target-specific dependencies are optional and referenced as dependency of some feature.

Take tokio as an example: The winapi dependency is scoped to cfg(windows), so we definitely don't need it on linux, but some of its features are referenced (i.e. the net feature depends on the winapi/namedpipeapi feature of winapi).

Dropping optional, target-specific dependencies that are referenced like this will lead to broken Cargo.toml files (i.e. cargo will not parse them correctly), unless you also drop their references from the features in which they are mentioned - you can look at our downstream patch for tokio how we need to handle that.

9 new commits added

  • config: allow wanted-features= in addition to unwanted-features=
  • Allow setting all-features= in rust2rpm.conf
  • Make "rust2rpm.conf" the normal configuration file
  • Filter out removed dependencies from [feature] lists
  • Automatically strip foreign dependencies
  • Add cfg.parse_and_evaluate()
  • Refactor the patching code to pass args lower in the call stack
  • docs: start adding docstrings to functions
  • Add parser and evaluator for rust cfg expressions
5 months ago

I added code to filter out the feature lists that match removed dependencies. For tokio, the result is identical to current patch (which I assume was generated out of a manual edit).

Fabio, since you're the biggest user of this, I'd appreciate a thorough review ;) I did what seemed the most reasonable, but I'm far from certain that this is the best option.

OK, I'll take a thorough look, then. If it does the same thing for tokio as my current patch, that's a good start. While I'm not sure that we can handle all edge cases automatically, that should not matter so long as there's an option to turn it off. However, since people are only ... well, human, it would be great to also have a patch-foreign = false|true flag for rust2rpm.conf to make this choice persistent for a given package.

For the rest of the review, I'll make inline comments. :)

"Wanted" features don't make any sense to me. Feature subpackages are always generated for all features, except if they are explicitly disabled with unwanted-features. The only use I would have for a "wanted" feature is if we have some optional features that we need to enable for our distro builds with -f foo,bar, but that only affects %cargo_{generate_buildrequires,build,install,test} flags, and not the actual feature subpackages that are generated in the RPM.

This seems to be a typo: faturesfeatures. We also usually put the features we disable on separate lines, not sure if the listify function actually accepts comma-separated features instead of whitespace-separated names.

I think we could drop the positive_negative_filter function, since there's actually only ever "negative" filtering, which is already in place.

Why do you explicitly override the default value to None here?

The store_false action has a default value of True and stores False in case it was supplied (and vice-versa for store_true):
https://docs.python.org/3/library/argparse.html#action

Do you actually need the value to not have the "default" value but an explicit None? That would be weird.

Also accepting rust2rpm.conf as a file name is nice, though this is probably a change that is independent of the "cfg parse" PR here.

(I also didn't realize _rust2rpm.conf was already an accepted file name, I always used .rust2rpm.conf, which often creates confusion because it's a "hidden" file ...)

Same here. "wanted" features are a no-op that doesn't make any sense, unless I'm stupid today.

You could take a look at the test cases I wrote for the cfg expression evaluator I wrote a while ago:
https://github.com/ironthree/cargoman/blob/master/src/eval.rs#L78

The test cases were "inspired" by cfg expressions that I actually came across when dealing with various packages.

Oh, now I definitely remember one edge case that my implementation of this did not handle well:

If a feature depends on target-specific dependencies or features of dependencies, it's not really clear what should happen if the resulting list of "feature dependencies" is empty after removing target-specific features + dependencies.

  • keep the feature, but with an empty list of dependencies,
  • strip the feature as well

Both can be correct, depending on the circumstances:

  1. There are crates that have target-generic features, like tokio. Those must not be removed, even if all target-specific feature dependencies are dropped from them.
  2. There's also crates that have something like a win feature that depends on all the windows-specific optional dependencies and features. They can be dropped, but don't need to be (and might be handled with an "unwanted-feature" flag instead).

So I think it's safer to assume that even if you strip all dependencies of a feature, to keep that feature by default, and only explicitly remove it as a packager (because there's no way to know programmatically).

I've started to split off some changes from this pretty big PR into separate changes, which should be easier to review and accept. c.f. PR #189

I've now split the addition of the "all-features" rust2rpm.conf setting into a separate PR as well: PR #191

"Wanted" features don't make any sense to me. Feature subpackages are always generated for all features, except if they are explicitly disabled with unwanted-features. The only use I would have for a "wanted" feature is if we have some optional features that we need to enable for our distro builds with -f foo,bar, but that only affects %cargo_{generate_buildrequires,build,install,test} flags, and not the actual feature subpackages that are generated in the RPM.

OK, I dropped the patch to add wanted-features.

Why do you explicitly override the default value to None here?

The store_false action has a default value of True and stores False in case it was supplied (and vice-versa for store_true):
https://docs.python.org/3/library/argparse.html#action

Do you actually need the value to not have the "default" value but an explicit None? That would be weird.

Yes, I want to have None if the option wasn't specified, and True or False if it was. (FWIW, I think that the argparse magic of changing the default value is a terrible thing. It's surprising and completely unnecessary, since bool(None) == False.)

You could take a look at the test cases I wrote for the cfg expression evaluator I wrote a while ago:
https://github.com/ironthree/cargoman/blob/master/src/eval.rs#L78

The test cases were "inspired" by cfg expressions that I actually came across when dealing with various packages.

I looked at the list, but it seems that the cases I already added are either identical or cover essentially the same things.

So I think it's safer to assume that even if you strip all dependencies of a feature, to keep that feature by default, and only explicitly remove it as a packager (because there's no way to know programmatically).

I think the current PR might be doing this already. Do you have some example to check the behaviour?

rebased onto 37645d7

a month ago

The first few commits are the same as #195.

"Wanted" features don't make any sense to me. Feature subpackages are always generated for all features, except if they are explicitly disabled with unwanted-features. The only use I would have for a "wanted" feature is if we have some optional features that we need to enable for our distro builds with -f foo,bar, but that only affects %cargo_{generate_buildrequires,build,install,test} flags, and not the actual feature subpackages that are generated in the RPM.

^@alebastr: you requested this. Can you comment more?

rebased onto e95fa12

a month ago

I reworked this quite a bit. Now two patches are generated: the automatic patch, called foo-automatic.diff, and the human-created patch, called foo-manual.diff. I tested this with a bunch of different crates, incl. tokio, and it seems DTRT.

rebased onto 5bf0041

a month ago

This is now rebased on top #200. With the tests it's easier to see what this series actually does.

"Wanted" features don't make any sense to me. Feature subpackages are always generated for all features, except if they are explicitly disabled with unwanted-features. The only use I would have for a "wanted" feature is if we have some optional features that we need to enable for our distro builds with -f foo,bar, but that only affects %cargo_{generate_buildrequires,build,install,test} flags, and not the actual feature subpackages that are generated in the RPM.

^@alebastr: you requested this. Can you comment more?

The most obvious case is #110, which I've seen on multiple crates.

There's an optional feature cli (which indeed has a subpackage, but that does not help in the slightest), and rust2rpm is not smart enough to figure out that the binaries depend on the feature.
So it happily generates a broken spec and you have to patch everything with -f cli or regenerate with --all-features to generate a complete set of dynamic buildrequires and make it build.
And I imagine the case where you want to avoid using all-features -- there could be broken/missing/outdated deps for yet another optional feature.

More crate examples: vcsgraph, cargo-lock.

To be fair, Fabio's comments convinced me that wanted-features is a hack and we need to handle bin.required-features instead (which would translate to something like -f cli anyways).

Not necessarily. Just the initial implementation was wrong.

We could still add something like an "enabled-features = foo bar" setting that translates to -f foo,bar, which would solve this use case, wouldn't it?

Yes, but if the only valid usecase for enabled-features is to work around lack of support for bin.required-features, it would be better to fix the [[bin]] section parsing.
Less confusion for the packagers, less documentation needed (...if your package build breaks at %build because of missing dependencies or at %install because of missing binary files, try to regenerate with enabled-features = cli, etc).

Although, on a second thought I recall that there are a few packages where we have to enable optional features (bindgen) by default. Could be another valid scenario.

I'll return to #110 separately.

rebased onto 61e7b5f

a month ago

Updated with some whitespace cleanups.

OK, let's merge this. I'm pretty sure that changes will be required, but it'll probably be easier to do this on the main branch so that people can submit pull requests.

Commit 2b28554 fixes this pull-request

Pull-Request has been merged by zbyszek

a month ago

Pull-Request has been merged by zbyszek

a month ago