Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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 nirs, almusil 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