openSUSE / obs-service-go_modules

OBS Source Service to download, verify, and vendor Go module dependency sources
GNU General Public License v2.0
19 stars 18 forks source link

Rework the service to better work with obs_scm #19

Closed darix closed 2 years ago

darix commented 2 years ago
  1. autodetect function only handled tar files which used the same compression as you want for the vendor tarball

  2. files with our _service: prefix were not handled at all.

  3. moved the basename parsing into a helper function as it now also had to remove the _service prefix

  4. now that we have to handle not just tarballs but also cpios, switched the archive handling to

    https://github.com/Changaco/python-libarchive-c

    python3-libarchive-c as package name

  5. refactored get_archive_extension function to also handle cpios and mapping our parameter names to names that libarchive expects.

  6. moved all the code that was sprinkled between the functions into the if name == "main" block so it only runs if we actually execute the script

Fixes #7

jfkw commented 2 years ago

Thanks for the PR, will review. The changes do appear to address several gaps following the configurable compression PR merge.

Gathering some information organized by your numbered objectives:

  1. Yes, will merge. With the configurable compression, compression detection of source and vendor archives should be independent.
  2. Agree in principle that we would like to support, but the _service: prefixed server-side intermediate files are a bit of an unknown to me at present. I haven't maintained a project that uses them and would like to understand the use case (for Go) better before we commit to support. Given that most Go applications are developed with upstream using git, what are the advantages to using _service: prefixes instead of a regular named archives? When preparing an OBS commit, can a local osc build for _service: be run with the identical behavior as it will have server side once checked in? Can you point to an OBS project (presumably non Go) that has a similar _service definition to those envisioned for Go so we can discuss how it fits with Go packages?
  3. Yes, will merge, seems to improve function granularity.
  4. I do have reservations about adding dependencies, and this dependency python-libarchive-c has the added complexity of the python C FFI. Only one other package in Factory diffoscope depends on it. python-libarchive-c does not appear to be packaged in SLE at this time, is that that correct? That would seem to be a gate for adding this feature. If we need support sooner, calling the necessary shell commands could be an option. We already to that for go mod. Are _service: prefix support and .obscpio format support coupled? IIUC, these features are used for server-side source service processing, and therefore would need to be installed on OBS instances. Do server-side source services install and run in the OBS distro/version environment or, does the source service also need to be installable by the build target repository, i.e. all the distros and versions supported as OBS build targets?
  5. Neutral to OK to merge a version of this with some changes. The PR does point out that we use a few globals, the log object, args and outdir. Creation of those could move under if __name__ == __main__. It seems like calls to get_archive_parameters() and basename_from_archive_name(archive) could happen in main(), will look more closely. I have been planning to change globals other than the log object (and eventually perhaps than one too) to locals and pass them as function arguments. Haven't done this yet though. So I don't think we should add any more globals here.

I have a moderate-diff change to commit, tag and release this week to fix an issue encountered one package. If you can rebase after that, we can see if there are parts of the PR we can merge sooner while continuing the larger python-libarchive-c dependency discussion.

Is there a workflow to collaborate on another user's PR branch before merging? I haven't found a good guide on accomplishing that, and it's led to some unreasonably long delays in addressing PRs on my part.