fedora-rust / rust2rpm

Clone
Source Code
GIT

#279 Allow non-interactive `rust2rpm -p`
Closed 2 months ago by decathorpe. Opened 6 months ago by lecris.
fedora-rust/ lecris/rust2rpm fix/278  into  main

Add --non-interactive flag
Cristian Le • 4 months ago  
67cb7a1
file modified
+3 -1 
@@ -113,10 +113,12 @@ 
 

      do_patch = args.patch or tomlconf.package_cargo_toml_patch_comments is not None
 

      do_patch_foreign = args.patch_foreign
 

      do_vendor = args.vendor or args.target == "epel8"
 

+     

+     non_interactive = args.non_interactive
 

  

      try:
 

          project, version, diffs, metadata, doc_files, license_files, is_local, vendor_tarball = process_project(
 

-             args.crate, args.version, do_patch, do_patch_foreign, args.store_crate, do_vendor
 

+             args.crate, args.version, do_patch, do_patch_foreign, args.store_crate, do_vendor, non_interactive
 

          )
 

      except NoVersionsError:
 

          log.error(f"No versions are available for crate {args.crate!r}.")

file modified
+5 
@@ -97,6 +97,11 @@ 
 

          help="Enable interactive mode",
 

      )
 

      parser.add_argument(
 

+         "--non-interactive",
 

+         action="store_true",
 

+         help="Run fully non-interactive",
 

+     )
 

+     parser.add_argument(
 

          "-V",
 

          "--vendor",
 

          action="store_true",

file modified
+5 -3 
@@ -216,6 +216,7 @@ 
 

      patch: bool,
 

      patch_foreign: bool,
 

      vendor: bool,
 

+     non_interactive: bool,
 

  ) -> tuple[str, str, tuple[Optional[list[str]], Optional[list[str]]], Metadata, list[str], list[str], Optional[str]]:
 

      if os.path.isdir(project):
 

          toml_path, doc_files, license_files = local_cargo_dir(project)
 
@@ -249,7 +250,7 @@ 
 

          version = package.version
 

  

          with toml_temp_copy(toml_path):
 

-             diffs = make_patches(name, package.version, patch, patch_foreign, toml_path, features)
 

+             diffs = make_patches(name, package.version, patch, patch_foreign, toml_path, features, non_interactive)
 

  

              # ensure metadata is up-to-date with changes from patches
 

              metadata = Metadata.from_cargo(toml_path)
 
@@ -299,6 +300,7 @@ 
 

      patch_foreign: bool,
 

      store_crate: bool,
 

      vendor: bool,
 

+     non_interactive: bool,
 

  ) -> tuple[
 

      str, str, tuple[Optional[list[str]], Optional[list[str]]], Metadata, list[str], list[str], bool, Optional[str]
 

  ]:
 
@@ -319,7 +321,7 @@ 
 

                  log.warn("The '--store-crate' flag has no effect for unpacked sources.")
 

  

              name, version, diffs, metadata, doc_files, license_files, vendor_tarball = process_project_local(
 

-                 project, version, patch, patch_foreign, vendor
 

+                 project, version, patch, patch_foreign, vendor, non_interactive
 

              )
 

              return name, version, diffs, metadata, doc_files, license_files, True, vendor_tarball
 

  
@@ -354,7 +356,7 @@ 
 

          package = metadata.packages[0]
 

          version = package.version
 

          features = package.get_feature_names()
 

-         diffs = make_patches(name, version, patch, patch_foreign, toml_path, features)
 

+         diffs = make_patches(name, version, patch, patch_foreign, toml_path, features, non_interactive)
 

  

          # ensure metadata is up-to-date with changes from patches
 

          metadata = Metadata.from_cargo(toml_path)

file modified
+24 -4 
@@ -1,10 +1,12 @@ 
 

  import ast
 

+ import glob
 

  from datetime import datetime, timezone
 

  from difflib import unified_diff
 

  import re
 

  import os
 

  import shutil
 

  import subprocess
 

+ import sys
 

  from typing import Optional
 

  

  from rust2rpm import cfg, log
 
@@ -169,7 +171,7 @@ 
 

  

  

  def make_patches(
 

-     name: str, version: str, patch: bool, patch_foreign: bool, toml_path: str, features: set[str]
 

+     name: str, version: str, patch: bool, patch_foreign: bool, toml_path: str, features: set[str], non_interactive: bool
 

  ) -> tuple[Optional[list[str]], Optional[list[str]]]:
 

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

  
@@ -191,6 +193,7 @@ 
 

      #    If this succeeded, remove the original Cargo.toml file, write the
 

      #    changed contents back to disk, and generate a diff.
 

      # 2) if requested, open Cargo.toml in an editor for further, manual changes
 

+     #    unless non_interactive is True, in which case the last patch is attempted
 

      #
 

      # The changes from *both* steps must be reflected in the Cargo.toml file
 

      # that ends up on disk after this function returns. Otherwise, the
 
@@ -213,9 +216,26 @@ 
 

          toml_after = toml_before
 

  

      if patch:
 

-         # open editor for Cargo.toml
 

-         editor = detect_editor()
 

-         subprocess.check_call([editor, toml_path])
 

+         if non_interactive:
 

+             # Try to apply the provided patch with more relaxed `fuzz`
 

+             # then re-create the patch that %autosetup can use with fuzz=0
 

+             try:
 

+                 # TODO: use the actual {base_name}-fix-metadata.diff instead of glob
 

+                 patch_file = glob.glob("*-fix-metadata.diff")

I hate this approach, but in order to get the base_name here, I need to refactor thest to a class so that I can have access to the variable more arbitrarily.

 
+                 if len(patch_file) != 1:
 

+                     log.error("Could not find the appropriate fix-metadata.diff patch file")
 

+                     sys.exit(1)
 

+                 patch_file = patch_file[0]
 

+                 subprocess.check_call(["patch", "--strip=1", toml_path, patch_file])
 

+             except subprocess.CalledProcessError:
 

+                 log.error(
 

+                     f"Could not apply the patch file, please review it manually"
 

+                 )
 

+                 sys.exit(1)
 

+         else:
 

+             # Otherwise, open editor for Cargo.toml
 

+             editor = detect_editor()
 

+             subprocess.check_call([editor, toml_path])
 

  

          with open(toml_path) as file:
 

              toml_after2 = file.read()

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

Not sure how to deal with the tests

I'm not sure this is the best way to add this feature ... what is up with tests?

what is up with tests?

I tried to navigate in the tests there to see where to add, but I couldn't find relevant tests to do so.

I'm not sure this is the best way to add this feature

Sure, anything catches your eye?

I was deliberating if we should use the -p file as-is instead of recreating *-fix-metadata.diff each time, but that would rather clash with extra-patches.

Other than that, I didn't want to do any refactoring here since I don't know what is planned on that front.
Edited 6 months ago by lecris

what is up with tests?

I tried to navigate in the tests there to see where to add, but I couldn't find relevant tests to do so.

It's possible that there's just no tests for the patching module yet. It's a bit difficult to test this since it's supposed to be interactive.

I'm not sure this is the best way to add this feature

Sure, anything catches your eye?

I was deliberating if we should use the -p file as-is instead of recreating *-fix-metadata.diff each time, but that would rather clash with extra-patches.

I'm not sure I understand. I would rather try to re-apply the existing patch and re-write it instead of keeping a patch that likely doesn't apply for the new version. extra-patches has nothing to do with this.

Other than that, I didn't want to do any refactoring here since I don't know what is planned on that front.

No refactoring is currently planned.

I would rather try to re-apply the existing patch and re-write it instead of keeping a patch that likely doesn't apply for the new version

Ack, I forgot about that flow. I looked a bit for solutions that can apply the results of difflib.unified_diff but I couldn't find anything within difflib library. What approach do you consider? My thinking is to try a subprocess.run("patch") with default --fuzz and pass the content of the patch_file

I haven't looked into it yet. I usually use git to manage my patches, not diff / patch directly :(

One thing I know is that we don't want to use --fuzz=0 like it is used for %autosetup, if we want to try to re-format the patch, e.g. if there is just a trivial new line added.

But about the erroring if the patch fails instead of falling back to editor, all's good with that? That would make it usable for non-interactive workflow like that.

1 new commit added

  • Try to apply the patch and re-format it when needed
6 months ago

I still think the -p argument should not take an argument. The filename will always be the same anyway.

What I imagined this workflow would look like:

  • try to re-apply existing -fix-metadata.diff patch
  • if the patch applies without problems: refresh patch and write it out
  • if the patch does not apply:
    • in non-interactive mode: fail
    • in interactive mode (default): open editor

So basically my idea would be to add a --non-interactive switch to rust2rpm to control the behaviour if the patch fails to apply.

EDIT: Scratch that, even if the existing patch still applies, sometimes you need to change it. So there would need to be a way to do that too ...
Edited 6 months ago by decathorpe

So basically:

  • if --non-interactive and -p
    • try to apply *-fix-metadata.diff
    • if it fails fpr any reason, including the file is not there than fail
  • if --non-interactive without -p: so far nothing special
  • if -p: current workflow as-is

What about the short option for --non-interactive if any?
Edited 6 months ago by lecris

I don't know. It's been a while since I worked on this part of the code, so I'll need some time to figure out the best solution here.

Oh, FTR, found a critical flaw with using -p as and optional parameter rust2rpm -p ./atuin-18.3.0. Technically not allowed for workspaces anyway but still, it's a total no-go approach.

Yeah, mixing flags with optional parameters and positional-only arguments won't work anyway.

rebased onto 07af5a7
4 months ago

I hate this approach, but in order to get the base_name here, I need to refactor thest to a class so that I can have access to the variable more arbitrarily.

Regarding the --non-interactive flag, would this fly (except for the glob part, that would need more refactoring)? The option kinda clashes with --interactiveflag, but at least the workflow is more on how you would want it?

Edit: One "broken?" behavior is that if you rust2rpm --non-interactive and you don't pass the -p, it will still go through and find and use the patch file. I guess this is behavior change to the last release where it would report failure if you didn't have the -p flag, but now it opens the editor.
Edited 4 months ago by lecris

Pull-Request has been closed by decathorpe
2 months ago
Metadata
None
No Tags
Changes Summary 4
+3 -1
file changed
rust2rpm/__main__.py
+5 -0
file changed
rust2rpm/cli.py
+5 -3
file changed
rust2rpm/crate.py
+24 -4
file changed
rust2rpm/patching.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.