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

Set package version without requiring to do manual steps before building #46

Closed
rinigus opened this issue Apr 29, 2021 · 26 comments · May be fixed by #47
Closed

Set package version without requiring to do manual steps before building #46

rinigus opened this issue Apr 29, 2021 · 26 comments · May be fixed by #47

Comments

@rinigus
Copy link

rinigus commented Apr 29, 2021

In the condition where droid-hal-%{device}-kernel-modules is absent in the target, rpmspec cannot parse droid-hal-XXX-img-boot.spec as included droid-hal-device-img-boot.inc doesn't define Version in these conditions.

I suggest to ensure that localver macro has always some value defined. For example, after https://github.com/mer-hybris/hybris-initrd/blob/master/droid-hal-device-img-boot.inc#L54 add something like

%if "%{localver}" == ""
%define localver 0.0.1
%endif

Not an expert in SPEC, please feel free to suggest something better.

Context: processing SPECs in the repository covering multiple devices with the different kernels.

@Thaodan
Copy link
Contributor

Thaodan commented Apr 29, 2021

You should install the kernel modules before you build this package and don't reuse targets/snapshots for different device variants.

@rinigus
Copy link
Author

rinigus commented Apr 29, 2021

Yes, the package is installed before the build. But, it shouldn't be needed for just parsing RPM SPEC using rpmspec while figuring out build requirements and other properties. Obviously, version will be wrong, but current state leads to complete failure.

Notice the presence of _obs_build_project in this SPEC. Which is workaround for other condition as Version is replaced at OBS anyway.

Notice that during the build, version is determined correctly as a correct device kernel modules are pulled in.

So, the issue comes when having xz2/xz2c/xz3 in the same shared RPMs folder, in the config as in OBS. As these devices require different kernels. I cannot have them installed at the same time if they have the same version due to the conflicts.

@martyone
Copy link
Contributor

@rinigus Try if processing the spec with rpm --eval first helps:

rpm --eval "$(< path/to/a.spec)" > temp.spec
rpmspec -q --buildrequires temp.spec

For me it worked with a regular spec file.

@rinigus
Copy link
Author

rinigus commented Apr 29, 2021

@martyone , this doesn't choke anymore, but result is not super useful (and I keep this as an example of some weird rpm magic with "$(< rpm/droid-hal-akari-img-boot.spec)"):

PlatformSDK $ rpm --eval "$(< rpm/droid-hal-akari-img-boot.spec)" > a.spec
PlatformSDK $ cat a.spec 
%include initrd/droid-hal-device-img-boot.inc
PlatformSDK $ rpmspec -q --buildrequires a.spec 
warning: line 59: Possible unexpanded macro in: Name:       droid-hal-%{device}-img-boot
warning: line 123: Possible unexpanded macro in: %package -n droid-hal-%{device}-img-recovery
coreutils
cpio
cryptsetup
droid-hal-%{device}-kernel
droid-hal-%{device}-kernel-modules
droid-hal-%{device}-tools
e2fsprogs
fsarchiver
hw-ramdisk
initrd-helpers
initrd-logos
lvm2
oneshot
openssh-clients
openssh-server
python
sed
yamui

So, here we get unprocessed %{device} in the requirements, unfortunately.

By making the proposed change, no such issues will occur.

@Thaodan
Copy link
Contributor

Thaodan commented Apr 29, 2021

Notice the presence of _obs_build_project in this SPEC. Which is workaround for other condition as Version is replaced at OBS anyway.

Yes this done because the version is tagged in the device repo, the proper fix would be to use the version from the git tag locally too in case it can't be determined earlier.

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

Yes this done because the version is tagged in the device repo, the proper fix would be to use the version from the git tag locally too in case it can't be determined earlier.

I think it will be a case of over-engineering and could have more negative aspects without any positive. Iff we set the version by git tag in this case, we will pretend that this version carries some information. In this particular case, it is useless, as the repository version stays the same while kernels are updated. So, by adding a code determining the version from git would lead next developers to think that it is correct and could work instead. Which it does not.

In case of OBS, I presume that it is using its own SPEC parser. rpmspec would refuse to parse the generated SPEC due to https://github.com/mer-hybris/hybris-initrd/blob/master/droid-hal-device-img-boot.inc#L99 which is still empty when passing _obs_build_project macro as an argument (without that macro it fails on empty version).

My proposed patch makes SPEC parsed by regular rpmspec which is a goal over here.

As this left unanswered and to avoid confusion:

You should install the kernel modules before you build this package and don't reuse targets/snapshots for different device variants.

TBuilder is not force resetting target snapshots before building each package. It is a bit hidden in the code, but such reset occurs before the build for a package is started. After successful build, snapshot is reset with force option and the next package is built.

@Thaodan
Copy link
Contributor

Thaodan commented Apr 30, 2021

Yes this done because the version is tagged in the device repo, the proper fix would be to use the version from the git tag locally too in case it can't be determined earlier.

I think it will be a case of over-engineering and could have more negative aspects without any positive. Iff we set the version by git tag in this case, we will pretend that this version carries some information. In this particular case, it is useless, as the repository version stays the same while kernels are updated. So, by adding a code determining the version from git would lead next developers to think that it is correct and could work instead. Which it does not.

In case of OBS, I presume that it is using its own SPEC parser. rpmspec would refuse to parse the generated SPEC due to https://github.com/mer-hybris/hybris-initrd/blob/master/droid-hal-device-img-boot.inc#L99 which is still empty when passing _obs_build_project macro as an argument (without that macro it fails on empty version).

Nope tar_git overrides the version so 0.1 gets replaced by the correct version that's why I said go with using git tag (see official devices).

@martyone
Copy link
Contributor

I must be blind. Yesterday I was able to reproduce the issue, now I am not. Nothing changed to my environment and the terminal's scrollback provides clear evidence it used to fail before.

a.spec:

%define kernelver %(rpm -q --queryformat '%%{Version}' kernel-headers)
%define localver %(echo "%{kernelver}" |cut -d . -f 1,2)

Name:       foobar
Version:    %{localver}
Release:    1%{?dist}
Summary:    Foo Bar
License:    BSD
Source0:    %{name}-%{version}.tar.bz2

BuildRequires:  bazbaz-%{device}

%description
Foo Bar

b.spec:

%define device mydev
%include a.spec

query it:

$ rpmspec -q b.spec 
foobar-3.18-1.i586
$ rpmspec -q --buildrequires b.spec 
bazbaz-mydev

and also:

$ rpmspec -q --buildrequires a.spec      # <--- it's a.spec!
bazbaz-%{device}        # <-- so this is expected!

What am I doing wrong? :)

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

@martyone: the section I have issue with is

%define kernelversion %(rpm -q --qf '[%%{version}-%%{release}]' droid-hal-%{device}-kernel)
%define kernelmodulesversion %(rpm -q --qf '[%%{version}-%%{release}]' droid-hal-%{device}-kernel-modules)
%define kernelver %(rpm -ql droid-hal-%{device}-kernel-modules | sort | grep /lib/modules/ | head -1 | rev | cut -d '/' -f 1 | rev)
%define localver %(echo "%{kernelver}" | cut -d '-' -f1 | cut -d '+' -f1)

that one requires droid-hal-%{device}-kernel-modules to be installed, not kernel-headers as you have. Not sure where your section is coming from

@Thaodan:

Nope tar_git overrides the version so 0.1 gets replaced by the correct version that's why I said go with using git tag (see official devices).

does it mean that you tag https://github.com/mer-hybris/droid-hal-img-boot-sony-seine for every kernel update? As that is a repo that we build over here. I am probably missing something with OBS in this case...

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

$ rpmspec -q --buildrequires a.spec      # <--- it's a.spec!
bazbaz-%{device}        # <-- so this is expected!

As I forgot to reply to this: I am parsing with rpmspec the spec that will be built. So, in context of official devices, it would be https://github.com/mer-hybris/droid-hal-img-boot-sony-seine/blob/master/rpm/droid-hal-pdx201-img-boot.spec . This is done from the project directory (https://github.com/mer-hybris/droid-hal-img-boot-sony-seine/blob/master/) as in

rpmspec -q rpm/droid-hal-pdx201-img-boot.spec

to ensure that all included files are included properly. In this case, %{device} is defined and I will get proper build requirement.

@martyone
Copy link
Contributor

@rinigus yeah that made the difference in my case - I worked with the kernel-modules package before, then I switched to the kernel-headers in order to be able testing different aspects of the evaluation and forgot about.

It's really a chicken-egg problem. Wouldn't this work?

-%if 0%{?_obs_build_project:1}
-Version:    0.0.1
+# This needs droid-hal-%%{device}-kernel-modules but that is only pulled in as BuildRequires.
+%if 0%{!?localver:1}
+Version:    999.999.999.999
 %else
 Version:    %{localver}
 %endif

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

we also have {localver} used for Provides: kernel = %{version} line. As far as I understood RPM SPEC and tested, localver would be defined at that point and %if 0%{!?localver:1} would evaluate as true even with the empty string as a contents. At least that what I've got last night while testing the patch.

So, to handle it in a simple manner, I propose to check if localver evaluates into empty string and fill it with something in this case. As in submitted PR #47

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

Damn, misread Provides statement. Probably your approach will work as well, but 0%{!?localver:1} would have to be replaced with something. Let me check it to be sure, sorry for rushing with reply

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

Found the reason for confusion: on Tama, I set Provides: kernel = %{localver} to be able to require that to match with DTBO package. In the commit ~2 years ago - sailfishos-sony-tama@d594179 - as it was, to my knowledge, the first SFOS device with DTBO.

So, for me, it is better to define localver to something meaningful. For upstream, I guess your approach would work after fixing %if statement

When using

Name:       droid-hal-%{device}-img-boot
Summary:    Kernel boot image for %{device}
#if 0%{?_obs_build_project:1}
#Version:    0.0.1
%if 0%{!?localver:1}
Version:    999.999.999.999
%else
Version:    %{localver}
%endif

it fails for me (error: line 66: Empty tag: Version:). With

Name:       droid-hal-%{device}-img-boot
Summary:    Kernel boot image for %{device}
#if 0%{?_obs_build_project:1}
#Version:    0.0.1   
%if "%{localver}" == ""
Version:    999.999.999.999
%else
Version:    %{localver}
%endif

it works.

To reduce the diff between Tama and upstream, would be nice to have localver redefined though. But, in principle, it is up to you. For Tama I need to keep separate hybris-init anyway as we have additional functionality in recovery (ability to take snapshots of filesystems and restore them).

@martyone
Copy link
Contributor

Found the reason for confusion: on Tama, I set Provides: kernel = %{localver} to be able to require that to match with DTBO package. In the commit ~2 years ago - sailfishos-sony-tama@d594179 - as it was, to my knowledge, the first SFOS device with DTBO.

Given that there is "Version: %{localver}", wouldn't this work equally?

Provides: kernel = %{version}

(Assumed you do not need to do rpmspec -q --provides and only determine provides from the binaries.)

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

It would for non-OBS builds. With OBS, you end up with the versions like 4.9.230+git1-1.18.2 which messes up requirement for DTBO.

And provides are determined from RPM binaries, as we have some that are generated (pkgconfig ones, for example).

@Thaodan
Copy link
Contributor

Thaodan commented Apr 30, 2021

@Thaodan:

Nope tar_git overrides the version so 0.1 gets replaced by the correct version that's why I said go with using git tag (see official devices).

does it mean that you tag https://github.com/mer-hybris/droid-hal-img-boot-sony-seine for every kernel update? As that is a repo that we build over here. I am probably missing something with OBS in this case...

The version is set but rebuild with a new pkgrel every time a dependency updates.

@Thaodan Thaodan changed the title droid-hal-device-img-boot.inc cannot be parsed without droid-hal-%{device}-kernel-modules installed Set package version without requiring to do manual steps before building Apr 30, 2021
@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

@Thaodan - I guess the new title is more of a side-effect of the requested change :) .

@Thaodan
Copy link
Contributor

Thaodan commented Apr 30, 2021 via email

@rinigus
Copy link
Author

rinigus commented Apr 30, 2021

Note that git is not available in SDK by default. So, even git cannot be used by rpmspec at present to figure out the version.

In principle, I think RPM SPEC should be readable in a form that it passes rpmspec without dependencies and using only tools that are installed in SDK by default. Otherwise it is a bug that should be fixed on SPEC side. How else are we expected to get requirements for the build?

@Thaodan
Copy link
Contributor

Thaodan commented Apr 30, 2021 via email

@martyone
Copy link
Contributor

martyone commented May 3, 2021

When mb2 is used for building, the Version gets substituted by mb2 based on Git tag the same way as tar_git does (the '--fix-version' option is implied when sources are under Git control). So, provided that the tags match the tags under the kernel package, there is no need for working with git at .spec level and the following should just work:

-%if 0%{?_obs_build_project:1}
-Version:    0.0.1
-%else
-Version:    %{localver}
-%endif
+Version:    0

Leaving Version: 0 is better than 0.0.1 as it suggests something happens with the version. Note that Release needs to remain at 1, setting it to 0 would be invalid.

@rinigus
Copy link
Author

rinigus commented May 3, 2021

@martyone: git tag for that specific package is not usually the kernel version. So, use of localver is justified perfectly in this case. But we can replace 0.0.1 by 0 as an indication. Assuming nothing else breaks with it

@martyone
Copy link
Contributor

martyone commented May 3, 2021

hm, I was told the tags do match :) Anyway it feels odd when sources are tagged with different version than the version used for binaries, doesn't? So if they really don't match (in some cases), maybe it's a good opportunity to align them. But take me with a grain of salt, I have no idea of the actual tagging practice here. All I am working with is the bit of information I got indirectly, so my suggestions may be completely wrong.

@rinigus
Copy link
Author

rinigus commented May 3, 2021

So, let's check:

Problem is that we don't have to change that particular package (droid-hal-img-boot) with the change in kernel version. So, it is easy to get the situation where droid-hal-img-boot repo stays the same while kernel is updated. But, you would want it to indicate the kernel version as well.

Similar state is with PA module packages, if I understand correctly. There, version is taken from PA itself while release indicates state of the module.

@martyone
Copy link
Contributor

Fixed with PR #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants