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

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

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux