search

mer-hybris / hybris-initrd

initrd generation submodule for flashable Sailfish OS image based boot images and recovery images.
2 stars 17 forks source link

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

Closed rinigus closed 2 years ago

rinigus commented 3 years ago

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 commented 3 years ago

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

rinigus commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago 
$ 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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 - https://github.com/sailfishos-sony-tama/hybris-initrd/commit/d5941791c9414d3ed08c2f66f647cfdbafa22ef5 - 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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.

rinigus commented 3 years ago

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

Thaodan commented 3 years ago

The requested change did not describe the actual issue. Just adding some version there to let RPM "parse" the right fix is a hack around the issue.

Requiring the install of dependencies prior build manually is hack in packaging terms even ignoring the target might being unclean or any other applied hacks somewhere else.

We should find a fix for that just let mb2/tar_git set the version from git.

rinigus commented 3 years ago

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 commented 3 years ago

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? The packaged is tied to the kernel if you go further than this this package would be part of droid-hal-kernel but since that is not practically we need to use workarounds.

martyone commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

Fixed with PR #50