#94 [DRAFT] spectool: implement support for SourceScript
Closed a month ago by decathorpe. Opened 3 months ago by decathorpe.
Unknown source main  into  main

file modified
+1 -1
@@ -1,2 +1,2 @@

  [tool.black]

- line-length = 100

+ line-length = 120

file modified
+52 -9
@@ -22,6 +22,7 @@

  

  import argparse

  import os

+ import subprocess

  import tempfile

  import time

  from collections import OrderedDict
@@ -191,6 +192,14 @@

          help="output debug info, don't clean up when done",

      )

  

+     misc.add_argument(

+         "--safe",

+         action="store_const",

+         const=True,

+         default=False,

+         help="run in safe mode (i.e. do not run a detected SourceScript)",

+     )

+ 

      specfile = parser.add_argument("specfile", action="store")

  

      if argcomplete:
@@ -339,7 +348,7 @@

              self.print_patch(number, value)

  

      @staticmethod

-     def _get_file(value: str, directory: str, force: bool, dry: bool):

+     def _get_file(value: str, directory: str, force: bool, dry: bool) -> bool:

          parsed = urlparse(value)

  

          if "#" not in value:
@@ -350,8 +359,7 @@

                  basename = basename.lstrip("/")

              except ValueError:

                  # multiple "#" characters inside

-                 print("Invalid URL:", value)

-                 return

+                 raise ValueError(f"Invalid URL: {value}")

  

          if parsed.scheme:

              if not dry:
@@ -361,6 +369,7 @@

                      print("Downloading: {}".format(value))

                      os.makedirs(directory, exist_ok=True)

                      really = get_file(value, path, force)

+ 

                      if really:

                          print("Downloaded: {}".format(basename))

  
@@ -379,11 +388,16 @@

              else:

                  print("Would have downloaded: {}".format(value))

  

-     def get_source(self, number: int, directory: str, force: bool, dry: bool, value: str = None):

+             return True

+ 

+         else:

+             return False

+ 

+     def get_source(self, number: int, directory: str, force: bool, dry: bool, value: str = None) -> bool:

          if not value:

              value = self.sources[number]

  

-         self._get_file(value, directory, force, dry)

+         return self._get_file(value, directory, force, dry)

  

      def get_patch(self, number: int, directory: str, force: bool, dry: bool, value: str = None):

          if not value:
@@ -391,14 +405,40 @@

  

          self._get_file(value, directory, force, dry)

  

-     def get_sources(self, directory: str, force: bool, dry: bool):

+     def get_sources(self, directory: str, force: bool, dry: bool, safe: bool):

+         any_local = False

          for number, value in self.sources.items():

-             self.get_source(number, directory, force, dry, value)

+             any_local = any_local or not self.get_source(number, directory, force, dry, value)

+ 

+         if any_local:

+             self.run_source_script(safe)

  

      def get_patches(self, directory: str, force: bool, dry: bool):

          for number, value in self.patches.items():

              self.get_patch(number, directory, force, dry, value)

  

+     def run_source_script(self, safe: bool):

+         with open(self.path) as file:

+             contents = file.read()

+ 

+         source_script = None

+         for line in contents.splitlines():

+             if line.startswith("# SourceScript:"):

+                 source_script = line.split()[2]

+                 break

+ 

+         if source_script and safe:

+             print("Not running detected SourceScript in safe mode.")

+             return

+ 

+         if source_script:

+             version = self.spec.sourceHeader[rpm.RPMTAG_VERSION]

+             print(f"Running script to generate sources: {source_script} {version}")

+ 

+             cmd = [f"./{source_script}", version]

+             ret = subprocess.run(cmd)

+             ret.check_returncode()

+ 

  

  def main() -> int:

      args = get_args()
@@ -484,6 +524,7 @@

      if args["get_files"]:

          force = args["force"]

          dry = args["dry_run"]

+         safe = args["safe"]

  

          if args["directory"] and args["sourcedir"]:

              print("Conflicting requests for download directory.")
@@ -504,10 +545,12 @@

                      print("No patch with number '{}' found.".format(number))

                      continue

  

-                 spec.get_source(number, directory, force, dry)

+                 local = not spec.get_source(number, directory, force, dry)

+                 if local:

+                     spec.run_source_script(safe)

  

          elif args["sources"] and not args["patch"]:

-             spec.get_sources(directory, force, dry)

+             spec.get_sources(directory, force, dry, safe)

  

          if args["patch"]:

              numbers = split_numbers(args["patch"])

This commit adds support for automatically running an external script
for generating Source files (for example, if the upstream sources
need to be cleaned of unwanted files and repackaged).

This functionality is triggered if any specified Source file is a
local file, or when downloading all sources. In this case, the spec
file contents are parsed and if a line matches the following format

# SourceScript: path-to-script.sh

The script is executed, with the package version supplied as an
argument:

./path-to-script.sh $VERSION

In environments where running user-supplied code is not desirable,
the "--safe" CLI parameter can be used to turn off executing detected
SourceScript files.

Hello, there is also relevant discussion here:
https://pagure.io/packaging-committee/issue/1132

The problem (aimed to solve in the issue) in this solution is the filename (or path) in comment, which should be just a comment and not interpreted / run / bear any data for automation.

It would be nice to have this standartized / recommended for packaging (regardless of whether it's in the specfile, or accompanying file).


FTR, this is my approach (already in 'production' for some time): https://github.com/pvalena/theprototype/blob/main/pkgs/sources.sh

@pvalena you realize that I'm the current author of spectool, the person who submitted this PR, and a member of the packaging committee? ...

Of course, once we agree on a way to do this that everybody is happy with, the packaging committee can decide whether to mandate this standardized new way of defining steps to manually create source files. And given that there already need to be reproducible steps to create such files, this should not be hard to formalize.

I'm really starting to wonder why we're not adding these things to fedpkg. These are Fedora-isms and quirks of specifically how Fedora package lifecycling works.

For example, rpmdev-spectool is used by OBS as a source service mechanism, but only as one of them. There are many others. And other distributions enhance their workflow tools to do extra things. These are all handled as part of osc. And mgarepo does its own thing, as does abf, etc.

I'm really starting to wonder why we're not adding these things to fedpkg.

I don't think fedpkg is the right place, either. We'd need this functionality for packages that are maintained outside dist-git-style repos (i.e. for things that are still in the package review process, or for packages that are only pushed to COPR), and fedpkg doesn't work right in those circumstances at all.

This means we'd need a tool that's not specific to dist-git (like spectool), possibly with an external config file (so something like kentauros, which I have been using for building nightly snapshots of elementary packages in COPR for years).

COPR uses rpkg, which is the library underpinning fedpkg, so functionality added there would benefit it too. But I see your point. The idea of parsing spec file comments scares me, though...

@pvalena you realize that I'm the current author of spectool, the person who submitted this PR, and a member of the packaging committee? ...

Yes, sorry if I made the impression I'm somehow opposing the proposal. I'm in favor of any and all implementations which suit / try to help with packaging.

Of course, once we agree on a way to do this that everybody is happy with, the packaging committee can decide whether to mandate this standardized new way of defining steps to manually create source files. And given that there already need to be reproducible steps to create such files, this should not be hard to formalize.

Exactly, that's just what I was trying to point out, as I was probably missing a consensus / proper conclusion on that, and also the 'comments' in spec file. I would be happy to see that formalized :) in any form that's fitting.

I was also hoping to add context / my current use / of what's being done, so it would be (maybe? ) more aligned, and if possible, It'd be also great for me to be able to use this functionality.

COPR uses rpkg, which is the library underpinning fedpkg, so functionality added there would benefit it too. But I see your point. The idea of parsing spec file comments scares me, though...

I'd be in favor of enabling fedpkg to work without a repository created (and adding more functionality as well), as I currently end up working around it by adding git remote manually and acting as the package already exists.
e.g.: https://koji.fedoraproject.org/koji/taskinfo?taskID=86888702

Given that there's disagreement what we want and where we want it, I'm closing this draft PR.

If the eventual concensus will be that an external config file for spectool would be fine, I can look into implementing support for that.

Pull-Request has been closed by decathorpe

a month ago
Metadata