NAK: [Plucky/Unstable PATCH 2/2] UBUNTU: [Packaging] remove unneeded prerequisites of $(DEBIAN)/control.stub

[Agathe Porte]

2024-11-21 22:03 CET, Masahiro Yamada:
> On Wed, Nov 20, 2024 at 6:05 PM Agathe Porte <agathe.porte at canonical.com> wrote:
> >
> > 2024-11-20 03:22 CET, Masahiro Yamada:
> > > $(DEBIAN)/control.stub is marked as .PHONY.
> > >
> > > There is no need to specify version-controlled files as prerequisites
> > > for a phony target since they already exist.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiro.yamada at canonical.com>
> > > ---
> > >
> > >  debian/rules | 8 +-------
> > >  1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > diff --git a/debian/rules b/debian/rules
> > > index 600e7bab2150..d1a6c2a78521 100755
> > > --- a/debian/rules
> > > +++ b/debian/rules
> > > @@ -180,13 +180,7 @@ UBUNTU_COMPATIBLE_SIGNING=$(shell grep -qx ' *Subject: C = GB, ST = Isle of Man,
> > >
> > >  # Misc stuff
> > >  .PHONY: $(DEBIAN)/control.stub
> > > -$(DEBIAN)/control.stub:                              \
> > > -             debian/scripts/control-create           \
> > > -             $(control_files)                        \
> > > -             debian/canonical-revoked-certs.pem      \
> > > -             debian/control.d/flavour-module.stub    \
> > > -             $(DEBIAN)/changelog                     \
> > > -             $(wildcard $(DEBIAN)/control.d/*)
> > > +$(DEBIAN)/control.stub: debian/canonical-revoked-certs.pem
> > >       for i in $(control_files); do                                           \
> > >         cat $$i;                                                              \
> > >         echo "";                                                              \
> >
> > I think the prerequisites are still useful in the case one of them is
> > deleted by accident for whatever reason: the rule will fail early saying
> > that the required file is missing.
> >
> > It also allows to see what the rule depends on, even if it is PHONY and
> > will run everytime.
> >
> > See the below example Makefile:
> >
> >         .PHONY: foo
> >         foo: in.txt
> >                 @echo Hello
> >
> > When run without in.txt:
> >
> >         make foo
> >         make: *** No rule to make target 'in.txt', needed by 'foo'.  Stop.
> >
> > I think we should keep this dependency list to fail early.
> >
> > Maybe we could just drop the $(wildcard $(DEBIAN)/control.d/*) since
> > it will always "be true" as a dependency and brings no real value.
> >
> > What do you think?
> 
> 
> In general, I tend to drop check-in files from prerequisites
> because if they were missing for some reason, the build
> rule would fail for "No such file or directory" error anyway.
> 
> However, in this case, the build rule is overly long, and
> multiple commands are piped together.
> 
> Here,
> 
>   for i in $(control_files); do cat $$i; echo ""; done |  ...
> 
> If any file in $(control_files) were deleted accidentally,
> you would see some error message, but Make may continue
> running and finish successfully because the error status
> on the left ride of the pipe operator is ignored.
> 
> I am fine with keeping this code as-is for now.
> (but, this part will be cleaned up more if my later patches are accepted)

NAKing this one this you sent a v2 without the prerequisites drop.
Would be happy to review more cleanups in your later patches.

Thanks.