RFC: dpkg patch to using -ffile-prefix-map

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: dpkg patch to using -ffile-prefix-map

Vagrant Cascadian-4
With gcc 8 being the default compiler in debian now, we should be able
to use -ffile-prefix-map, which should handle *some* of the cases that
BUILD_PATH_PREFIX_MAP is intended to solve. It definitely won't help
with things that embed the gcc commandline into arguments.

But since we've been unable to convince gcc on the merits of
BUILD_PATH_PREFIX_MAP using an environment variable, rebasing the gcc
patches to support it takes a lot of effort, perhaps we should explore
other options.

I've got a dpkg patch which makes use of -ffile-prefix-map. I haven't
found a good test case yet... package that fails to build reproducibly
due to using __FILE__, __BASE_FILE__, or __builtin_FILE() and builds
fast enough that it's easy to test (holger suggested trying "dtkwm", but
I haven't had a chance to try yet). Figured I'd at least publish my
patch to get some review and/or testers.

The patch largely just copy-and-pastes the -fdebug-prefix-map code,
though arguably could obsolete it entirely, since -ffile-prefix-map
effectivly sets both -fdebug-prefix-map and -fmacro-prefix-map. But
having the two features independently allows enabling or disabling one
or the other easily for now.

If nothing else, carrying a patch on dpkg builds *much* faster than gcc,
and rebasing it periodically will be a lot less effort. Though if it
works, hopefully we can get it into dpkg directly.

Thanks!


live well,
  vagrant


commit a500874714fc8f7865d897653d8a33ab3ce7d7ff
Author: Vagrant Cascadian <[hidden email]>
Date:   Thu Jul 26 16:49:28 2018 +0800

    Add support for -ffile-prefix-map, which enables both
    -fdebug-prefix-map and -fmacro-prefix-map to also support common path
    related macros.

diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
index 1e8f24397..315d8b9b8 100644
--- a/scripts/Dpkg/Vendor/Debian.pm
+++ b/scripts/Dpkg/Vendor/Debian.pm
@@ -101,6 +101,7 @@ sub _add_build_flags {
         reproducible => {
             timeless => 1,
             fixdebugpath => 1,
+            fixfilepath => 1,
         },
         sanitize => {
             address => 0,
@@ -218,6 +219,20 @@ sub _add_build_flags {
         }
     }
 
+    # Mask features that might have an unsafe usage.
+    if ($use_feature{reproducible}{fixfilepath}) {
+        require Cwd;
+
+        $build_path = $ENV{DEB_BUILD_PATH} || Cwd::cwd();
+
+        # If we have any unsafe character in the path, disable the flag,
+        # so that we do not need to worry about escaping the characters
+        # on output.
+        if ($build_path =~ m/[^-+:.0-9a-zA-Z~\/_]/) {
+            $use_feature{reproducible}{fixfilepath} = 0;
+        }
+    }
+
     # Warn when the __TIME__, __DATE__ and __TIMESTAMP__ macros are used.
     if ($use_feature{reproducible}{timeless}) {
        $flags->append('CPPFLAGS', '-Wdate-time');
@@ -235,6 +250,19 @@ sub _add_build_flags {
         $flags->append('GCJFLAGS', $map);
     }
 
+    # Avoid storing the build path in the debug symbols and binaries
+    # when the  __FILE__, __BASE_FILE__, and __builtin_FILE() macros are used.
+    if ($use_feature{reproducible}{fixfilepath}) {
+        my $map = '-ffile-prefix-map=' . $build_path . '=.';
+        $flags->append('CFLAGS', $map);
+        $flags->append('CXXFLAGS', $map);
+        $flags->append('OBJCFLAGS', $map);
+        $flags->append('OBJCXXFLAGS', $map);
+        $flags->append('FFLAGS', $map);
+        $flags->append('FCFLAGS', $map);
+        $flags->append('GCJFLAGS', $map);
+    }
+
     ## Area: sanitize
 
     # Handle logical feature interactions.

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Guillem Jover
Hi!

On Tue, 2018-07-31 at 08:29:33 +0800, Vagrant Cascadian wrote:

> With gcc 8 being the default compiler in debian now, we should be able
> to use -ffile-prefix-map, which should handle *some* of the cases that
> BUILD_PATH_PREFIX_MAP is intended to solve. It definitely won't help
> with things that embed the gcc commandline into arguments.
>
> But since we've been unable to convince gcc on the merits of
> BUILD_PATH_PREFIX_MAP using an environment variable, rebasing the gcc
> patches to support it takes a lot of effort, perhaps we should explore
> other options.
>
> I've got a dpkg patch which makes use of -ffile-prefix-map. I haven't
> found a good test case yet... package that fails to build reproducibly
> due to using __FILE__, __BASE_FILE__, or __builtin_FILE() and builds
> fast enough that it's easy to test (holger suggested trying "dtkwm", but
> I haven't had a chance to try yet). Figured I'd at least publish my
> patch to get some review and/or testers.
Hah! Just several hours ago I was talking with Mattia over IRC about
the status of this, and ended up cooking the attached patch, which at
his request didn't merge, because he wanted to give it a try first.

And yeah, I also kind of understand gcc's upstream position, that if
you unconditionally embed all build flags into your resulting objects,
then they are really bound to be unreproducible, and as such that's
arguably a bug to fix there probably.

Also AIUI this divergence and the lack of forward porting is the
reason the repro buildds are currently stopped? If so I think getting
something going, even if it might produce some new (or old :)
reproducible problems is probably better than the current situation.

> The patch largely just copy-and-pastes the -fdebug-prefix-map code,
> though arguably could obsolete it entirely, since -ffile-prefix-map
> effectivly sets both -fdebug-prefix-map and -fmacro-prefix-map. But
> having the two features independently allows enabling or disabling one
> or the other easily for now.

I think the semantics in mine are slightly better? :)

> If nothing else, carrying a patch on dpkg builds *much* faster than gcc,
> and rebasing it periodically will be a lot less effort. Though if it
> works, hopefully we can get it into dpkg directly.

From my side, I see no problem with merging something like the
attached patch if it works (I've not even run the test suite on it I
think :).

Thanks,
Guillem

0001-Dpkg-Vendor-Debian-Add-fixfilepath-to-reproducible-f.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Chris Lamb -2
Hi Guillem,

> Also AIUI this divergence and the lack of forward porting is the
> reason the repro buildds are currently stopped?

(Indeed — see https://reproducible-builds.org/blog/posts/170/, 4th
paragraph…)

> From my side, I see no problem with merging something like the
> attached patch if it works (I've not even run the test suite on it I
> think :)

Thanks for all your work on this.


Best wishes,

--
      ,''`.
     : :'  :     Chris Lamb
     `. `'`      [hidden email] / chris-lamb.co.uk
       `-

Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Vagrant Cascadian-4
In reply to this post by Guillem Jover
On 2018-07-31, Guillem Jover wrote:

> On Tue, 2018-07-31 at 08:29:33 +0800, Vagrant Cascadian wrote:
>> I've got a dpkg patch which makes use of -ffile-prefix-map. I haven't
>> found a good test case yet... package that fails to build reproducibly
>> due to using __FILE__, __BASE_FILE__, or __builtin_FILE() and builds
>> fast enough that it's easy to test (holger suggested trying "dtkwm", but
>> I haven't had a chance to try yet). Figured I'd at least publish my
>> patch to get some review and/or testers.
>
> Hah! Just several hours ago I was talking with Mattia over IRC about
> the status of this, and ended up cooking the attached patch, which at
> his request didn't merge, because he wanted to give it a try first.
Heh.


> And yeah, I also kind of understand gcc's upstream position, that if
> you unconditionally embed all build flags into your resulting objects,
> then they are really bound to be unreproducible, and as such that's
> arguably a bug to fix there probably.

I get that argument, though I fear that becomes an eternal game of
whack-a-mole.


> Also AIUI this divergence and the lack of forward porting is the
> reason the repro buildds are currently stopped? If so I think getting
> something going, even if it might produce some new (or old :)
> reproducible problems is probably better than the current situation.

Agreed.


>> The patch largely just copy-and-pastes the -fdebug-prefix-map code,
>> though arguably could obsolete it entirely, since -ffile-prefix-map
>> effectivly sets both -fdebug-prefix-map and -fmacro-prefix-map. But
>> having the two features independently allows enabling or disabling one
>> or the other easily for now.
>
> I think the semantics in mine are slightly better? :)

Surely! Glad to see you've done it properly.


>> If nothing else, carrying a patch on dpkg builds *much* faster than gcc,
>> and rebasing it periodically will be a lot less effort. Though if it
>> works, hopefully we can get it into dpkg directly.
>
> From my side, I see no problem with merging something like the
> attached patch if it works (I've not even run the test suite on it I
> think :).

Great!


> --- a/scripts/Dpkg/Vendor/Debian.pm
> +++ b/scripts/Dpkg/Vendor/Debian.pm
> @@ -100,6 +100,7 @@ sub _add_build_flags {
>          },
>          reproducible => {
>              timeless => 1,
> +            fixfilepath => 0,
>              fixdebugpath => 1,
>          },
>          sanitize => {
So by default, it would be disabled initially, and then we could
explicitly enable this for the reproducible builds test framework? After
it proves to be working and useful and not disruptive, I presume we
would enable it by default?


live well,
  vagrant

signature.asc (847 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Guillem Jover
On Tue, 2018-07-31 at 10:40:08 +0800, Vagrant Cascadian wrote:
> On 2018-07-31, Guillem Jover wrote:
> > On Tue, 2018-07-31 at 08:29:33 +0800, Vagrant Cascadian wrote:
> > And yeah, I also kind of understand gcc's upstream position, that if
> > you unconditionally embed all build flags into your resulting objects,
> > then they are really bound to be unreproducible, and as such that's
> > arguably a bug to fix there probably.
>
> I get that argument, though I fear that becomes an eternal game of
> whack-a-mole.

Right. I guess there's also an argument there, that if you are using a
blacklist then maybe you are doing it wrong? Or doing it at all? Dunno
really. :)

> > --- a/scripts/Dpkg/Vendor/Debian.pm
> > +++ b/scripts/Dpkg/Vendor/Debian.pm
> > @@ -100,6 +100,7 @@ sub _add_build_flags {
> >          },
> >          reproducible => {
> >              timeless => 1,
> > +            fixfilepath => 0,
> >              fixdebugpath => 1,
> >          },
> >          sanitize => {
>
> So by default, it would be disabled initially, and then we could
> explicitly enable this for the reproducible builds test framework? After
> it proves to be working and useful and not disruptive, I presume we
> would enable it by default?

Yeah. That was at Mattia's request too. I'm not sure I'd mind much
setting it by default from the get go. But it might be wiser to let
the repro buildds crunch on this for a bit in case unrelated things
break. :)

Thanks,
Guillem

Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Holger Levsen-2
On Tue, Jul 31, 2018 at 04:54:09AM +0200, Guillem Jover wrote:
> > > --- a/scripts/Dpkg/Vendor/Debian.pm
> > > +++ b/scripts/Dpkg/Vendor/Debian.pm
[...]

thanks for your work on this, Guillem! (and Vagrant of course too!)

Not sure we attributed Guillem corrently on this in
https://reproducible-builds.org/blog/posts/171 (and can't check
currently as offline on the way back from DebConf18 and haven't updated
that git repo before I left...)

> > So by default, it would be disabled initially, and then we could
> > explicitly enable this for the reproducible builds test framework? After
> > it proves to be working and useful and not disruptive, I presume we
> > would enable it by default?
> Yeah. That was at Mattia's request too. I'm not sure I'd mind much
> setting it by default from the get go.

cool too!

> But it might be wiser to let
> the repro buildds crunch on this for a bit in case unrelated things
> break. :)

sure! We've been running this code since last Saturday... so in a while
we should know for sure :)


--
cheers,
        Holger

-------------------------------------------------------------------------------
                    holger@(debian|reproducible-builds).org

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: dpkg patch to using -ffile-prefix-map

Vagrant Cascadian-4
On 2018-08-07, Holger Levsen wrote:

> On Tue, Jul 31, 2018 at 04:54:09AM +0200, Guillem Jover wrote:
>> > So by default, it would be disabled initially, and then we could
>> > explicitly enable this for the reproducible builds test framework? After
>> > it proves to be working and useful and not disruptive, I presume we
>> > would enable it by default?
>> Yeah. That was at Mattia's request too. I'm not sure I'd mind much
>> setting it by default from the get go.
>
> cool too!
>
>> But it might be wiser to let
>> the repro buildds crunch on this for a bit in case unrelated things
>> break. :)
>
> sure! We've been running this code since last Saturday... so in a while
> we should know for sure :)
Which has been over a week now, though slightly modified to default to
enabling fixfilepath.

I've seen it fix at least one reproducibility issue(zbackup), and have
seen the correct flag passed in builds, so I can confirm that it's
working. I haven't seen obvious examples where it breaks anything,
though admittedly haven't looked very closely.

It'd be great to at least see it in dpkg in a default disabled state
soon, and then the test infrastructure can set the environment variable
DEB_BUILD_MAINT_OPTIONS=reproducible=+fixfilepath (or reproducible=+all
? I *think* that's the right syntax?) so the test infrastructure to
enables it...

Would it be safe to set that in the test infrastructure even when dpkg
has no support for it; does dpkg ignore unknown features?


The less conservative option would, of course, be to enable it by
default in sid... :)


live well,
  vagrant

signature.asc (847 bytes) Download Attachment