Skip to content

Conversation

@mz-pdm
Copy link
Member

@mz-pdm mz-pdm commented Aug 11, 2022

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 mz-pdm requested review from almusil, nirs and tinez as code owners August 11, 2022 15:15
@mz-pdm mz-pdm self-assigned this Aug 11, 2022
@mz-pdm mz-pdm requested a review from galitf August 15, 2022 12:18
@mz-pdm
Copy link
Member Author

mz-pdm commented Aug 17, 2022

ping

@michalskrivanek
Copy link
Member

lgtm

@mz-pdm mz-pdm requested a review from aesteve-rh August 18, 2022 08:18
@tinez
Copy link
Member

tinez commented Aug 18, 2022

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:

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

@mz-pdm
Copy link
Member Author

mz-pdm commented Aug 18, 2022

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.

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
Copy link
Member Author

mz-pdm commented Sep 5, 2022

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 d9dc07f: "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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants