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

[Uladzimir Bely <ubely@ilbers.de>]

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.

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. 

-- 
Uladzimir Bely