Re: [PATCH v5 07/12] sbuild: support of shell exports from dpkg_runbuild_prepend

[Jan Kiszka <jan.kiszka@siemens.com>]

On 02.02.22 10:13, Uladzimir Bely wrote:
> In the email from Wednesday, 2 February 2022 11:52:29 +03 user Jan Kiszka 
> wrote:
>> On 02.02.22 09:46, Moessbauer, Felix (T CED SES-DE) wrote:
>>>> -----Original Message-----
>>>> From: isar-users@googlegroups.com <isar-users@googlegroups.com> On
>>>> Behalf Of Jan Kiszka
>>>> Sent: Wednesday, February 2, 2022 7:57 AM
>>>> To: Uladzimir Bely <ubely@ilbers.de>; isar-users@googlegroups.com
>>>> Subject: Re: [PATCH v5 07/12] sbuild: support of shell exports from
>>>> dpkg_runbuild_prepend
>>>>
>>>> On 01.02.22 19:41, Uladzimir Bely wrote:
>>>>> In the email from Tuesday, 1 February 2022 21:09:07 +03 user Jan Kiszka
>>>>
>>>> wrote:
>>>>>> On 01.02.22 18:00, Uladzimir Bely wrote:
>>>>>>> Many of recipes often use shell exports done in dpkg_run_prepend, so
>>>>>>> that this changed environment is used during build.
>>>>>>>
>>>>>>> While sbuild is performed in isolated environment, we need a way to
>>>>>>> pass these variables to it. This is done by storing environment
>>>>>>> before dpkg_runbuild (after prepare_build) and finding just before
>>>>>>> the actual build what was changed or added.
>>>>>>>
>>>>>>> Signed-off-by: Uladzimir Bely <ubely@ilbers.de>
>>>>>>> ---
>>>>>>>
>>>>>>>  meta/classes/dpkg.bbclass | 18 ++++++++++++++++++
>>>>>>>  1 file changed, 18 insertions(+)
>>>>>>>
>>>>>>> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
>>>>>>> index 66db7ec5..c252e9b3 100644
>>>>>>> --- a/meta/classes/dpkg.bbclass
>>>>>>> +++ b/meta/classes/dpkg.bbclass
>>>>>>> @@ -29,12 +29,30 @@ do_install_builddeps[lockfiles] +=
>>>>>>> "${REPO_ISAR_DIR}/isar.lock">  addtask devshell after
>>>>>>> do_install_builddeps
>>>>>>>
>>>>>>> +DPKG_PREBUILD_ENV_FILE="${WORKDIR}/dpkg_prebuild.env"
>>>>>>> +
>>>>>>> +do_prepare_build_append() {
>>>>>>> +    env > ${DPKG_PREBUILD_ENV_FILE} }
>>>>>>> +
>>>>>>>
>>>>>>>  # Build package from sources using build script
>>>>>>>  dpkg_runbuild() {
>>>>>>>  
>>>>>>>      E="${@ isar_export_proxies(d)}"
>>>>>>>      E="${@ isar_export_ccache(d)}"
>>>>>>>      export PARALLEL_MAKE="${PARALLEL_MAKE}"
>>>>>>>
>>>>>>> +    env | while read -r line; do
>>>>>>> +        # Filter the same lines
>>>>>>> +        grep -q "^${line}" ${DPKG_PREBUILD_ENV_FILE} && continue
>>>>>>> +        # Filter some standard variables
>>>>>>> +        echo ${line} | grep -q "^HOME=" && continue
>>>>>>> +        echo ${line} | grep -q "^PWD=" && continue
>>>>>>> +
>>>>>>> +        var=$(echo "${line}" | cut -d '=' -f1)
>>>>>>> +        value=$(echo "${line}" | cut -d '=' -f2-)
>>>>>>> +        sbuild_export $var "$value"
>>>>>>> +    done
>>>>>>> +
>>>>>>>
>>>>>>>      profiles=$(grep "DEB_BUILD_PROFILES" ${SBUILD_CONFIG} | tail -n1
>>>>>>>      |
>>>>>>>      cut -d "'" -f 4) if [ ${ISAR_CROSS_COMPILE} -eq 1 ]; then
>>>>>>>      
>>>>>>>          profiles="${profiles} cross nocheck"
>>>>>>
>>>>>> So, this basically decouples "Avoid using shell environment during
>>>>>> the build" and similar conversions downstream from this series,
>>>>>> right? It's indeed good to have a compat path now.
>>>>>>
>>>>>> However, what are the patterns we want to push? Avoiding exports?
>>>>>> Then we should probably warn here that this compat path should be
>>>>>> avoided and might be removed in the future.
>>>>>>
>>>>>> If we want to keep both, we can probably leave several recipes alone
>>>>>> in that other series.
>>>>>>
>>>>>> Jan
>>>>>
>>>>> You might be right. Current solution doesn't force users to avoid
>>>>> using exports while they are silently supported.
>>>>>
>>>>> The first internal solution was keeping dpkg_build_export function but
>>>>> also passing custom shell exports (from dpkg_runbuild_prepend) to
>>>>> sbuild environment. And a warning about migration was shown.
>>>>>
>>>>> Later I removed this to make new patchset less invasive for downstreams.
>>>>> Anyway, if we agree that "dpkg_build_export" (like API function) is
>>>>> more preferable, I can get back this solution with warnings.
>>>>
>>>> This is not necessarily what I'm suggesting. Your conversion series works
>>>> without that API, primarily using template. We also have build profiles.
>>>> But we first of all need to define what we actually want.
>>>>
>>>> The benefit of pushing things into the build env, rather than feeding it
>>>> from the recipe, is likely that the resulting deb-src of such generated
>>>> packages can actually be used for rebuilding independently of Isar. So
>>>> there is likely value in deprecating classic export.
>>>
>>> Using environment variables to control the build in Debian/rules is
>>> generally discouraged (except for DEB_BUILD_PROFILES and
>>> DEB_BUILD_OPTIONS). But even for these variables, semantics are
>>> documented [1] and we / an ISAR user should strictly follow them when
>>> packaging.
>>>
>>> There are also reasons why you want to build a package in a clean
>>> environment (e.g. to avoid accidentally setting CC or CXX to icc or
>>> clang). Hence, I highly vote for not forwarding the environment into the
>>> sbuild build environment.
>> ...without a warning about a grace period during that we will still do
>> that to allow downstream migration. Then we have a plan, I would say.
>>
>> Jan
> 
> To make things clear, there are now three options:
> 
> 1) (Current) Just support passing shell exports user done in 
> dpkg_runbuild_prepend to (s)build environment. Users of downstreams are not 
> notified, so they continue using these exports. 
> 
> 2) Continue support custom shell exports by passing them to (s)build env, BUT 
> show warnings, that user should use some 'dpkg_build_export'. In general, this 
> doesn't differ much from the previous approach.
> 
> 3) Continue support custom shell exports by passing them to (s)build env, BUT 
> show warnings, that this stuff should be converted to templates. After some 
> period, stop supporting exports.

Templates or build profiles.

> 
> 4) Doesn't support shell exports passing to (s)build env, so it will 
> immediately break the builds.
> 
> As I understand correctly, the way (3) is the best solution. 
> 

That is also my understanding.

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux