[libvirt] [dockerfiles PATCH v2 2/4] refresh: Learn how to deal with the project's name

[Andrea Bolognani]

On Wed, 2019-07-31 at 15:29 +0200, Fabiano Fidêncio wrote:
> +    # PROJECTS is a dictionary of dictionaries.
> +    # The key is the project name, as present in the Dockerfile name and
> +    # the value is a dictionary containing the subprojects which the
> +    # dependencies should be installed together as the key and their value
> +    # being whether they support mingw builds or not.
> +    # This hack is needed till the moment libvirt-jenkins-ci treats mingw
> +    # builds in the very same way as cross-builds are treated.
> +    PROJECTS = {
> +        "libvirt" : {
> +            "libvirt" : True
> +        },
> +    }

Yeah, having to hack around it this way is pretty unfortunate since
it's quite clear that ultimately we want to start treating MinGW
builds the same way as cross-architecture builds, but that's a lot
more work that shouldn't block you from adding more CI to libosinfo.

> +        for project in Dockerfile.PROJECTS:
> +            if stem.rfind(project) >= 0:
> +                self.project_name = project
> +                for subproject in Dockerfile.PROJECTS[project]:
> +                    self.projects += [subproject]
> +                break

We should report an error if we were unable to match any of the
known project names. Also ...

>          cross = stem.rfind(Dockerfile.CROSS)
>  
>          if cross >= 0:
> -            # If we found CROSS, then everything before it is the name of
> -            # the OS and everything after it the name of the architecture
> -            # we're targeting for cross-compilation
> -            self.os = stem[:cross]
> +            # If we found CROSS, then everything in between the project's
> +            # name and the cross is the name of the OS and everything after
> +            # it the name of the architecture we're targeting for
> +            # cross-compilation
> +            # Mind the project name has an extra '-', which is taken into
> +            # consideration when getting the OS
> +            self.os = stem[len(self.project_name) + 1:cross]
>              self.cross_arch = stem[cross + len(Dockerfile.CROSS):]
>          else:
> -            # Otherwise the entire stem is the name of the OS and there
> -            # is no cross-compilation architecture
> -            self.os = stem
> +            # Otherwise the name of the OS starts just after the project's
> +            # name and there is no cross-compilation architecture
> +            # Mind the project name has an extra '-', which is taken into
> +            # consideration when getting the OS
> +            self.os = stem[len(self.project_name) + 1:]
>              self.cross_arch = None

... having to do the same operation change in the two branches is
pretty nasty. I'm reviewing this without having refamiliarized myself
with the code, but can't you just do something like

  for project in Dockerfile.PROJECTS:
      if stem.rfind(project + "-") >= 0:
          self.project_name = project
          stem = stem[len(project) + 1:]
          ...

and avoid having to change any of the code below? Note that I added
a "-" to the string we pass to rfind(), since we need it to be there
or once again it means we're not dealing with well-formed file names
and should error out.

>          args += [
>              "libvirt-" + self.os,
> -            self.projects,
> +            ",".join(self.projects),
>          ]

This change is kind of separate, and in fact I had already proposed
it as its own patch[1] which you helpfully ACKed, so I've just pushed
it now.

Other than the few issues mentioned above, the changes look good and
so do the next two patches. Looking forward to v3 :)


[1] https://www.redhat.com/archives/libvir-list/2019-July/msg01159.html
-- 
Andrea Bolognani / Red Hat / Virtualization