search

oVirt / vdsm

The Virtual Desktop Server Manager
GNU General Public License v2.0
161 stars 202 forks source link

spec: Use vdsm.spec directly rather than generating it #296

Open mz-pdm opened 2 years ago

mz-pdm commented 2 years ago

We currently generate vdsm.spec from vdsm.spec.in. This means that whatever we want to do regarding building Vdsm packages, we must run auto* stuff first and having all the necessary tools available before we start with the building process, which is not always convenient or even possible.

The only thing that is replaced in vdsm.spec.in when generating vdsm.spec from it is @PACKAGE_RELEASE@. We can set 1 as its default value and use vdsm.spec instead of vdsm.spec.in directly. If the value needs to be different, it can be overridden using ‘rpmbuild --define’. This is what ‘rpm’ target in Makefile.am already does so the release is set as intended as long as ‘make rpm’ is used to build the rpm’s. Tagged releases work fine with the default value.

mz-pdm commented 2 years ago

ping

michalskrivanek commented 2 years ago

lgtm

tinez commented 2 years ago

Generally I'm ok with the taken direction, but could you elaborate on what problem are we specifically trying to solve? I think the same can be achieved by sed 's/@PACKAGE_RELEASE@/1' vdsm.spec.in > vdsm.spec if needed. Some thoughts I have on this PR:

mz-pdm commented 2 years ago

Generally I'm ok with the taken direction, but could you elaborate on what problem are we specifically trying to solve?

It should ease automated d/s builds but I think having a less or more directly usable .spec file in the repo is good generally. @galitf can provide details about the particular needs for d/s builds.

I think the same can be achieved by sed 's/@PACKAGE_RELEASE@/1' vdsm.spec.in > vdsm.spec if needed.

Yes (just a missing ending slash in s/.../.../?). But, @galitf, IIRC you need a .spec file in the upstream repo, right?

Some thoughts I have on this PR:

  • would it be beneficial to not require the presence of vdsm-release at all by having %{!?vdsm_release: %include vdsm-release} in the spec?

A very good idea!

  • in tarballs we already have a VERSION file, so seeing that one along with vdsm-version and vdsm-version.in makes things a bit confusing

Right, thanks for pointing this out.

@galitf may need to include additional lines in the spec file but I think this is a separate problem that can be solved either by adding another %include (although the %include in %{!?vdsm_release: %include vdsm-release} could be perhaps abused for this) or (preferably) by maintaining a modified .spec in a separate d/s branch.

mz-pdm commented 2 years ago

How about this simplified version? Does it miss anything we need?

As why we cannot generate the release dynamically from the spec file, it's explained in https://github.com/oVirt/vdsm/commit/d9dc07f3c422bc5cc4fcc80d98cea36cad1ad5c1: "A spec file doesn't know the location of the source directory when generating a SRPM and thus can't run pkg-version script from it."

This simplified version may not make @galitf particularly happy because it doesn't allow including additional stuff, but this can be resolved either by adding the lines to the .spec file from the repo or by another patch, based on the particular needs.