Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Hi!
On Tue, 2023-12-19 at 23:33:47 -0800, Steve Langasek wrote:
> On Sun, Jul 09, 2023 at 08:06:56PM +0200, Guillem Jover wrote:
> > I realized now that this cannot be set for CXXFLAGS as at least g++
> > will warn about that. And I've gone for now by depending on qa=+bug,
> > but I'm not sure whether that would cause too many regressions. OTOH
> > and AFAIK any such problem should be considered a genuine bug anyway,
> > so…
>
> > But if this is too much I guess I could split that warning into its
> > own feature and make both the qa=+bug and abi=+time64 depend on it
> > instead.
>
> Now that I've had a chance to look at the implementation, I am concerned
> about abi=+time64 implying qa=+bug, because I see that this turns on
> additional -Werror options beyond implicit-function-declaration:
>
> my @cfamilyflags = qw(
> array-bounds
> clobbered
> volatile-register-var
> );
> foreach my $warnflag (@cfamilyflags) {
> $flags->append('CFLAGS', "-Werror=$warnflag");
> $flags->append('CXXFLAGS', "-Werror=$warnflag");
> }
>
> While these may all be bugs, forcing the fixing of bugs unrelated to the
> time_t transition during the transition will inevitably slow down Debian
> development, so I am concerned about having such a dependency. We
> unfortunately also haven't done any archive rebuild analysis on this to
> understand how large the actual impact is (again, the focus has been on just
> getting the analysis right of which packages do have an ABI change).
>
> And while there's no specific timeline required on the Debian side yet, on
> the Ubuntu side we have a tight timeline to get this all done in the next
> couple of months so that it can be included in the 24.04 LTS release.
>
> The idea of splitting it into a separate feature seems ok, to avoid turning
> on unrelated -Werror options.
Ok, I'll prepare a patch this week to split it then.
> We still don't have a slot from the Release Team for when this can be
> landed, but following up to debian-devel with a complete analysis of the
> library ABIs is my next step before the end of year. Is this a change you
> think could be uploaded to dpkg on short notice? Or would you be amenable
> to an NMU, if you're unavailable for an upload?
Once the discussion is settled and the plan agreed, I think that will
also include agreeing on a date for all the involved uploads. TBH, I
don't expect that to be on short notice given all the parties that
might need to coordinate this, but I have no problem planning and
preparing an upload for a specific pre-agreed date. (And in case that
for some weird reason it would end up being on short notice I should
be able to manage too, I guess. :)
I also assume that with "this change" you refer to flipping the default
plus the -Werror flag stuff and not just the latter.
Regards,
Guillem
Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Hi Guillem,
Coming back to this after a hiatus. (In the intervening time the focus has
been getting the library ABI analysis right; now coming back around to
looking at the toolchain.)
On Sun, Jul 09, 2023 at 08:06:56PM +0200, Guillem Jover wrote:
> > There is analysis pending, unfortunately 90% of the -dev packages have been
> > analyzed leaving 90% to go (~1600 -dev packages that require fixes to be
> Ack, thanks. Is that last % supposed to be 10%? Otherwise I think I'm
> missing something :).
A reference to https://en.wikipedia.org/wiki/Ninety%E2%80%93ninety_rule
> > > > From 5a861d19b1610ae82bf95e6c5142a3365436fbd2 Mon Sep 17 00:00:00 2001
> > > > From: Steve Langasek
> > > > Date: Fri, 2 Jun 2023 14:30:20 +
> > > > Subject: [PATCH 1/3] lfs and time64 are no longer "future", call them
> > > > "feature" instead
> > > Ah, I actually had implemented locally the alias with your original
> > > suggestion of "abi"! :) Using "feature" here seems would be rather
> > > more confusing as these are called feature flags, and feature areas.
> > > I'll try to push the stuff I've got queued locally during the weekend,
> > > then I can rebase these patches on a branch or similar.
> > As far as I can tell, this hasn't been pushed anywhere yet?
> Right, sorry got entangled in a local branch with random stuff, but
> rebased that and added on top now several other fixes including these
> changes or ones similar in intention. See at:
> https://git.hadrons.org/git/debian/dpkg/dpkg.git/log/?h=next/time64-default
> (Beware that I might rebase that branch, before merging it, although I
> think I'll start pushing some of the foundation work into main already.)
> > Did you reach a decision here? Anything you'd like from me to move this
> > forward?
> I realized now that this cannot be set for CXXFLAGS as at least g++
> will warn about that. And I've gone for now by depending on qa=+bug,
> but I'm not sure whether that would cause too many regressions. OTOH
> and AFAIK any such problem should be considered a genuine bug anyway,
> so…
> But if this is too much I guess I could split that warning into its
> own feature and make both the qa=+bug and abi=+time64 depend on it
> instead.
Now that I've had a chance to look at the implementation, I am concerned
about abi=+time64 implying qa=+bug, because I see that this turns on
additional -Werror options beyond implicit-function-declaration:
my @cfamilyflags = qw(
array-bounds
clobbered
volatile-register-var
);
foreach my $warnflag (@cfamilyflags) {
$flags->append('CFLAGS', "-Werror=$warnflag");
$flags->append('CXXFLAGS', "-Werror=$warnflag");
}
While these may all be bugs, forcing the fixing of bugs unrelated to the
time_t transition during the transition will inevitably slow down Debian
development, so I am concerned about having such a dependency. We
unfortunately also haven't done any archive rebuild analysis on this to
understand how large the actual impact is (again, the focus has been on just
getting the analysis right of which packages do have an ABI change).
And while there's no specific timeline required on the Debian side yet, on
the Ubuntu side we have a tight timeline to get this all done in the next
couple of months so that it can be included in the 24.04 LTS release.
The idea of splitting it into a separate feature seems ok, to avoid turning
on unrelated -Werror options.
We still don't have a slot from the Release Team for when this can be
landed, but following up to debian-devel with a complete analysis of the
library ABIs is my next step before the end of year. Is this a change you
think could be uploaded to dpkg on short notice? Or would you be amenable
to an NMU, if you're unavailable for an upload?
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
signature.asc
Description: PGP signature
Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Hi!
On Wed, 2023-07-05 at 14:24:02 -0700, Steve Langasek wrote:
> On Fri, Jun 09, 2023 at 03:56:28AM +0200, Guillem Jover wrote:
> > On Mon, 2023-06-05 at 21:18:10 -0700, Steve Langasek wrote:
> > > Package: dpkg-dev
> > > Version: 1.21.22
> > > Tags: patch
> > > User: ubuntu-de...@lists.ubuntu.com
> > > Usertags: origin-ubuntu mantic patch
> > > The discussion on debian-devel around 64-bit time_t has died down, so I
> > > figure it's time to propose some patches to implement what's been
> > > discussed
> > > there.
>
> > Yeah, I'm not sure whether it has died off or it's just being slow.
>
> > > I'm not sure whether you were persuaded that i386 should stay with the
> > > current ABI, but anyway thought I would propose the patches and we could
> > > discuss further if necessary.
>
> > In any case I've tried to reply there. Also my impression was that
> > there was still some analysis pending (perhaps that was a wrong
> > impression though)?
>
> There is analysis pending, unfortunately 90% of the -dev packages have been
> analyzed leaving 90% to go (~1600 -dev packages that require fixes to be
Ack, thanks. Is that last % supposed to be 10%? Otherwise I think I'm
missing something :).
> compilable and therefore analyzable). I don't have any answer yet from the
> Release Team, so I'm not sure if we need this analysis to be completely done
> before starting the transition or if we can leave the long tail of packages
> with 1 or 2 reverse-build-dependencies to be figured out later.
I don't know. Perhaps ask on d-d?
> > > From 5a861d19b1610ae82bf95e6c5142a3365436fbd2 Mon Sep 17 00:00:00 2001
> > > From: Steve Langasek
> > > Date: Fri, 2 Jun 2023 14:30:20 +
> > > Subject: [PATCH 1/3] lfs and time64 are no longer "future", call them
> > > "feature" instead
>
> > Ah, I actually had implemented locally the alias with your original
> > suggestion of "abi"! :) Using "feature" here seems would be rather
> > more confusing as these are called feature flags, and feature areas.
>
> > I'll try to push the stuff I've got queued locally during the weekend,
> > then I can rebase these patches on a branch or similar.
>
> As far as I can tell, this hasn't been pushed anywhere yet?
Right, sorry got entangled in a local branch with random stuff, but
rebased that and added on top now several other fixes including these
changes or ones similar in intention. See at:
https://git.hadrons.org/git/debian/dpkg/dpkg.git/log/?h=next/time64-default
(Beware that I might rebase that branch, before merging it, although I
think I'll start pushing some of the foundation work into main already.)
> > > From 7eff8f89b32b6921a0d86c50c6c62154c6ddc96e Mon Sep 17 00:00:00 2001
> > > From: Steve Langasek
> > > Date: Fri, 2 Jun 2023 16:30:19 +
> > > Subject: [PATCH 3/3] Also emit -Werror=implicit-function-declaration for
> > > feature=+time64
> > >
> > > Per https://lists.debian.org/debian-devel/2023/05/msg00262.html et al.,
> > > missing glibc includes can cause packages to link to the wrong symbols,
> > > potentially causing crashes or misbehavior. Since functions that use
> > > time_t are fairly ubiquitous, there's a high risk of this happening for
> > > *some* package in Debian. Better to make all software with missing
> > > function declarations fail to build now, than to spend all cycle tracking
> > > down runtime bugs.
> > > ---
> > > scripts/Dpkg/Vendor/Debian.pm | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
> > > index 20d77fea1..803949024 100644
> > > --- a/scripts/Dpkg/Vendor/Debian.pm
> > > +++ b/scripts/Dpkg/Vendor/Debian.pm
> > > @@ -400,6 +400,8 @@ sub _add_build_flags {
> > >
> > > if ($flags->use_feature('feature', 'time64')) {
> > > $flags->append('CPPFLAGS', '-D_TIME_BITS=64');
> > > +$flags->append('CFLAGS',
> > > '-Werror=implicit-function-declaration');
> > > +$flags->append('CXXFLAGS',
> > > '-Werror=implicit-function-declaration');
> > > }
>
> > I'm not sure I like intermingling the different semantics here, if
> > necessary I'd rather make time64 forcibly enable another feature flag,
> > like it is done for lfs.
>
> > As I mentioned on the recent thread about the modern C stuff, I do
> > have a branch that adds these as part of a new qa=+c99 feature, but
> > that's too broad. :/
>
> >
> >
>
> > Maybe I could add a new feature area instead and add the flags
> > individually, and then make time64 also enable that other specific
> > feature. Hmmm.
>
> Did you reach a decision here? Anything you'd like from me to move this
> forward?
I realized now that this cannot be set for CXXFLAGS as at least g++
will warn about that. And I've gone for now by depending on qa=+bug,
but I'm not sure whether that would cause too many regressions. OTOH
and AFAIK
Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Hi,
I wanted to check in on the status of this.
On Fri, Jun 09, 2023 at 03:56:28AM +0200, Guillem Jover wrote:
> On Mon, 2023-06-05 at 21:18:10 -0700, Steve Langasek wrote:
> > Package: dpkg-dev
> > Version: 1.21.22
> > Tags: patch
> > User: ubuntu-de...@lists.ubuntu.com
> > Usertags: origin-ubuntu mantic patch
> > The discussion on debian-devel around 64-bit time_t has died down, so I
> > figure it's time to propose some patches to implement what's been discussed
> > there.
> Yeah, I'm not sure whether it has died off or it's just being slow.
> > I'm not sure whether you were persuaded that i386 should stay with the
> > current ABI, but anyway thought I would propose the patches and we could
> > discuss further if necessary.
> In any case I've tried to reply there. Also my impression was that
> there was still some analysis pending (perhaps that was a wrong
> impression though)?
There is analysis pending, unfortunately 90% of the -dev packages have been
analyzed leaving 90% to go (~1600 -dev packages that require fixes to be
compilable and therefore analyzable). I don't have any answer yet from the
Release Team, so I'm not sure if we need this analysis to be completely done
before starting the transition or if we can leave the long tail of packages
with 1 or 2 reverse-build-dependencies to be figured out later.
> Thanks for the patches!
> > From 5a861d19b1610ae82bf95e6c5142a3365436fbd2 Mon Sep 17 00:00:00 2001
> > From: Steve Langasek
> > Date: Fri, 2 Jun 2023 14:30:20 +
> > Subject: [PATCH 1/3] lfs and time64 are no longer "future", call them
> > "feature" instead
> Ah, I actually had implemented locally the alias with your original
> suggestion of "abi"! :) Using "feature" here seems would be rather
> more confusing as these are called feature flags, and feature areas.
> I'll try to push the stuff I've got queued locally during the weekend,
> then I can rebase these patches on a branch or similar.
As far as I can tell, this hasn't been pushed anywhere yet?
> > From 7eff8f89b32b6921a0d86c50c6c62154c6ddc96e Mon Sep 17 00:00:00 2001
> > From: Steve Langasek
> > Date: Fri, 2 Jun 2023 16:30:19 +
> > Subject: [PATCH 3/3] Also emit -Werror=implicit-function-declaration for
> > feature=+time64
> >
> > Per https://lists.debian.org/debian-devel/2023/05/msg00262.html et al.,
> > missing glibc includes can cause packages to link to the wrong symbols,
> > potentially causing crashes or misbehavior. Since functions that use
> > time_t are fairly ubiquitous, there's a high risk of this happening for
> > *some* package in Debian. Better to make all software with missing
> > function declarations fail to build now, than to spend all cycle tracking
> > down runtime bugs.
> > ---
> > scripts/Dpkg/Vendor/Debian.pm | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
> > index 20d77fea1..803949024 100644
> > --- a/scripts/Dpkg/Vendor/Debian.pm
> > +++ b/scripts/Dpkg/Vendor/Debian.pm
> > @@ -400,6 +400,8 @@ sub _add_build_flags {
> >
> > if ($flags->use_feature('feature', 'time64')) {
> > $flags->append('CPPFLAGS', '-D_TIME_BITS=64');
> > +$flags->append('CFLAGS', '-Werror=implicit-function-declaration');
> > +$flags->append('CXXFLAGS',
> > '-Werror=implicit-function-declaration');
> > }
> I'm not sure I like intermingling the different semantics here, if
> necessary I'd rather make time64 forcibly enable another feature flag,
> like it is done for lfs.
> As I mentioned on the recent thread about the modern C stuff, I do
> have a branch that adds these as part of a new qa=+c99 feature, but
> that's too broad. :/
>
>
> Maybe I could add a new feature area instead and add the flags
> individually, and then make time64 also enable that other specific
> feature. Hmmm.
Did you reach a decision here? Anything you'd like from me to move this
forward?
Thanks,
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
signature.asc
Description: PGP signature
Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Hi!
On Mon, 2023-06-05 at 21:18:10 -0700, Steve Langasek wrote:
> Package: dpkg-dev
> Version: 1.21.22
> Tags: patch
> User: ubuntu-de...@lists.ubuntu.com
> Usertags: origin-ubuntu mantic patch
> The discussion on debian-devel around 64-bit time_t has died down, so I
> figure it's time to propose some patches to implement what's been discussed
> there.
Yeah, I'm not sure whether it has died off or it's just being slow.
> I'm not sure whether you were persuaded that i386 should stay with the
> current ABI, but anyway thought I would propose the patches and we could
> discuss further if necessary.
In any case I've tried to reply there. Also my impression was that
there was still some analysis pending (perhaps that was a wrong
impression though)?
Thanks for the patches!
> From 5a861d19b1610ae82bf95e6c5142a3365436fbd2 Mon Sep 17 00:00:00 2001
> From: Steve Langasek
> Date: Fri, 2 Jun 2023 14:30:20 +
> Subject: [PATCH 1/3] lfs and time64 are no longer "future", call them
> "feature" instead
>
Ah, I actually had implemented locally the alias with your original
suggestion of "abi"! :) Using "feature" here seems would be rather
more confusing as these are called feature flags, and feature areas.
I'll try to push the stuff I've got queued locally during the weekend,
then I can rebase these patches on a branch or similar.
> From 7eff8f89b32b6921a0d86c50c6c62154c6ddc96e Mon Sep 17 00:00:00 2001
> From: Steve Langasek
> Date: Fri, 2 Jun 2023 16:30:19 +
> Subject: [PATCH 3/3] Also emit -Werror=implicit-function-declaration for
> feature=+time64
>
> Per https://lists.debian.org/debian-devel/2023/05/msg00262.html et al.,
> missing glibc includes can cause packages to link to the wrong symbols,
> potentially causing crashes or misbehavior. Since functions that use
> time_t are fairly ubiquitous, there's a high risk of this happening for
> *some* package in Debian. Better to make all software with missing
> function declarations fail to build now, than to spend all cycle tracking
> down runtime bugs.
> ---
> scripts/Dpkg/Vendor/Debian.pm | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
> index 20d77fea1..803949024 100644
> --- a/scripts/Dpkg/Vendor/Debian.pm
> +++ b/scripts/Dpkg/Vendor/Debian.pm
> @@ -400,6 +400,8 @@ sub _add_build_flags {
>
> if ($flags->use_feature('feature', 'time64')) {
> $flags->append('CPPFLAGS', '-D_TIME_BITS=64');
> +$flags->append('CFLAGS', '-Werror=implicit-function-declaration');
> +$flags->append('CXXFLAGS', '-Werror=implicit-function-declaration');
> }
I'm not sure I like intermingling the different semantics here, if
necessary I'd rather make time64 forcibly enable another feature flag,
like it is done for lfs.
As I mentioned on the recent thread about the modern C stuff, I do
have a branch that adds these as part of a new qa=+c99 feature, but
that's too broad. :/
Maybe I could add a new feature area instead and add the flags
individually, and then make time64 also enable that other specific
feature. Hmmm.
Thanks,
Guillem
Bug#1037136: dpkg-buildflags: 64-bit time_t by default
Package: dpkg-dev
Version: 1.21.22
Tags: patch
User: ubuntu-de...@lists.ubuntu.com
Usertags: origin-ubuntu mantic patch
Hi Guillem,
The discussion on debian-devel around 64-bit time_t has died down, so I
figure it's time to propose some patches to implement what's been discussed
there.
I'm not sure whether you were persuaded that i386 should stay with the
current ABI, but anyway thought I would propose the patches and we could
discuss further if necessary.
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer https://www.debian.org/
slanga...@ubuntu.com vor...@debian.org
From 5a861d19b1610ae82bf95e6c5142a3365436fbd2 Mon Sep 17 00:00:00 2001
From: Steve Langasek
Date: Fri, 2 Jun 2023 14:30:20 +
Subject: [PATCH 1/3] lfs and time64 are no longer "future", call them
"feature" instead
Recognize future= for backwards compatibility.
---
scripts/Dpkg/Vendor/Debian.pm | 32 ++--
scripts/t/Dpkg_BuildFlags.t | 2 +-
2 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
index 9285a61cf..f3d81bcc2 100644
--- a/scripts/Dpkg/Vendor/Debian.pm
+++ b/scripts/Dpkg/Vendor/Debian.pm
@@ -105,10 +105,14 @@ sub set_build_features {
# Default feature states.
my %use_feature = (
-future => {
+feature => {
lfs => 0,
time64 => 0,
},
+future => {
+lfs => -1,
+time64 => -1,
+},
qa => {
bug => 0,
canary => 0,
@@ -172,9 +176,17 @@ sub set_build_features {
($abi, $os, $cpu) = ('', '', '');
}
-## Area: future
+# compatibility: map future=[+-]lfs,time64 onto 'feature'
+if ((my $flag = $use_feature{future}{lfs}) != -1) {
+$use_feature{feature}{lfs} = $flag;
+}
+if ((my $flag = $use_feature{future}{time64}) != -1) {
+$use_feature{feature}{time64} = $flag;
+}
+
+## Area: feature
-if ($use_feature{future}{time64}) {
+if ($use_feature{feature}{time64}) {
# On glibc, new ports default to time64, old ports currently default
# to time32, so we track the latter as that is a list that is not
# going to grow further, and might shrink.
@@ -211,16 +223,16 @@ sub set_build_features {
if ($abi_bits != 32 or
not exists $time32_arch{$arch} or
$libc eq 'musl') {
-$use_feature{future}{time64} = 0;
+$use_feature{feature}{time64} = 0;
} elsif ($libc eq 'gnu') {
# On glibc 64-bit time_t support requires LFS.
-$use_feature{future}{lfs} = 1;
+$use_feature{feature}{lfs} = 1;
}
}
-if ($use_feature{future}{lfs}) {
+if ($use_feature{feature}{lfs}) {
if ($abi_bits != 32) {
-$use_feature{future}{lfs} = 0;
+$use_feature{feature}{lfs} = 0;
}
}
@@ -375,14 +387,14 @@ sub _add_build_flags {
$flags->append($_, $default_flags) foreach @compile_flags;
$flags->append('DFLAGS', $default_d_flags);
-## Area: future
+## Area: feature
-if ($flags->use_feature('future', 'lfs')) {
+if ($flags->use_feature('feature', 'lfs')) {
$flags->append('CPPFLAGS',
'-D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64');
}
-if ($flags->use_feature('future', 'time64')) {
+if ($flags->use_feature('feature', 'time64')) {
$flags->append('CPPFLAGS', '-D_TIME_BITS=64');
}
diff --git a/scripts/t/Dpkg_BuildFlags.t b/scripts/t/Dpkg_BuildFlags.t
index 850fe28b8..d64c54bfd 100644
--- a/scripts/t/Dpkg_BuildFlags.t
+++ b/scripts/t/Dpkg_BuildFlags.t
@@ -85,7 +85,7 @@ is($bf->get_origin('DPKGFLAGS'), 'env', 'flag has an env origin');
ok($bf->is_maintainer_modified('DPKGFLAGS'), 'prepend marked flag as maint modified');
my %known_features = (
-future => [ qw(
+feature => [ qw(
lfs
time64
) ],
--
2.40.1
From 02ea4e4b7b472754458a64f37f61712d55d25c91 Mon Sep 17 00:00:00 2001
From: Steve Langasek
Date: Fri, 2 Jun 2023 14:54:33 +
Subject: [PATCH 2/3] Enable time64 by default for all 32-bit archs, except for
i386
---
scripts/Dpkg/Vendor/Debian.pm | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
index f3d81bcc2..20d77fea1 100644
--- a/scripts/Dpkg/Vendor/Debian.pm
+++ b/scripts/Dpkg/Vendor/Debian.pm
@@ -107,7 +107,7 @@ sub set_build_features {
my %use_feature = (
feature => {
lfs => 0,
-time64 => 0,
+time64 => 1,
},
future => {
lfs => -1,
@@ -160,11 +160,6 @@ sub set_build_features {
my $opts_build =
