777018 - packager.mk: support "make package QUIET=1"

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

toolkit/mozapps/installer/packager.mk could benefit from a conditional $(QUIET) flag to decrease command stdout:

zip/unzip:
  ZIP_FLAGS   += -q
  UNZIP_FLAGS += -qq
  
Packager::Copy
  pass $7 or 0 in the debug field argument.

Add conditional redirection for commands:
  GENERATE_CACHE:
  OPTIMIZE_JARS_CMD:
    $(if $(QUIET),>/dev/null) &&

Comment 1

[Gregory Szorc [:gps]]

+1000

Comment 2

[Joey Armstrong [:joey]]

Attachment: support make package QUIET=1

Comment 3

[Joey Armstrong [:joey]]

Comment on attachment 647745 [details] [diff] [review]
support make package QUIET=1

Added a QUIET=1 conditional to squelch zip, unzip, GENERATE_CACHE, etc stdout.
When set append shell command silent options to std macros ZIP_FLAGS, UNZIP_FLAGS.  Pass them with $(ZIP), $(UNZIP) calls in the makefiles.
Define macros stdout=, stderr= for conditional output redirection.

Renamed QUIET= flag passed with jarmaker calls as JARMAKER_FLAGS so QUIET= can be used as a more generic option to throttle command output.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 647745 [details] [diff] [review]
support make package QUIET=1

Review of attachment 647745 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/makefiles/rules-quiet.mk
@@ +7,5 @@
> +stderr = $(if $(QUIET),>/dev/null)
> +stdout = $(if $(QUIET),>/dev/null)
> +
> +ZIP_FLAGS       += -q
> +UNZIP_FLAGS     += -qq

For 4 lines, 2 of which are never used (one of which is wrong), you probably don't need a separate file.

::: config/rules.mk
@@ +593,5 @@
>  endif # SUPPRESS_DEFAULT_RULES
>  
>  ifeq ($(filter s,$(MAKEFLAGS)),)
> +  ECHO := echo
> +  JARMAKER_FLAGS := $(strip $(subst -q,$(NULL),$(JARMAKER_FLAGS)))

Just don't set it. Nothing else defines this variable, and if a user does, it will override this value anyways.

@@ +598,3 @@
>  else
> +  ECHO := true
> +  JARMAKER_FLAGS += -q

:= instead of +=, for the same reason as above. Actually, I don't think you need a variable per program. Just have QUIET=-q EXTRA_QUIET=-qq, and use the appropriate one in the commands.

Comment 5

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 647745 [details] [diff] [review]
> support make package QUIET=1
> 
> Review of attachment 647745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/rules-quiet.mk
> @@ +7,5 @@
> > +stderr = $(if $(QUIET),>/dev/null)
> > +stdout = $(if $(QUIET),>/dev/null)
> > +
> > +ZIP_FLAGS       += -q
> > +UNZIP_FLAGS     += -qq
> 
> For 4 lines, 2 of which are never used (one of which is wrong), you probably don't need a separate file.

?? - Yes stderr was only added for completeness.  A quick grep of the patch will find references for the other three in packager.mk.

'-qq' is intentional if that option was the one appearing to be wrong, the argument is supported by unzip.

ps>  I'll merge them into rules.mk for now but more flags will likely need to be added later and can be split out then.  [ps] rules.mk really needs to be broken up and size reduced, esp to support unit testing.


> @@ +598,3 @@
> >  else
> > +  ECHO := true
> > +  JARMAKER_FLAGS += -q
> 
> := instead of +=, for the same reason as above. Actually, I don't think you need a variable per program. Just have QUIET=-q EXTRA_QUIET=-qq, and use the appropriate one in the commands.

One var per program will be needed for passing variant args to different commands (also has the benefit of arugment accumulation/overriding later on).

Trying to reuse a common flag like QUIET from "make QUIET=1 will barf on the call 'jarmaker 1' -vs- 'jarmaker -q'

Comment 6

[Gregory Szorc [:gps]]

Why don't we just default to quiet and make that the only option? If someone wants to see the content of an archive, they can just obtain the archive.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Joey Armstrong [:joey] from comment #5)
> '-qq' is intentional if that option was the one appearing to be wrong, the
> argument is supported by unzip.

The wrong one is stderr.

> > := instead of +=, for the same reason as above. Actually, I don't think you need a variable per program. Just have QUIET=-q EXTRA_QUIET=-qq, and use the appropriate one in the commands.
> 
> One var per program will be needed for passing variant args to different
> commands (also has the benefit of arugment accumulation/overriding later on).
> 
> Trying to reuse a common flag like QUIET from "make QUIET=1 will barf on the
> call 'jarmaker 1' -vs- 'jarmaker -q'

Then use some other name, but you don't need n variables for n programs that all take -q (except unzip, which takes -qq).

Comment 8

[Joey Armstrong [:joey]]

> > Trying to reuse a common flag like QUIET from "make QUIET=1 will barf on the
> > call 'jarmaker 1' -vs- 'jarmaker -q'
> 
> Then use some other name, but you don't need n variables for n programs that
> all take -q (except unzip, which takes -qq).

Using one variable inhibits the ability to perform operations like:
   make QUIET=1 ZIP_FLAGS=
to display output from a single command.  The alternative requires modifying source for what should be default command line behavior.

Comment 9

[(not currently active) Ted Mielczarek]

I think the right balance between flexibility and "too many options" is just to have a single toggle for quiet/verbose. If someone wants just the output of a single command they can pipe to a file and look for it.

Comment 10

[Joey Armstrong [:joey]]

Attachment: support make package QUIET=1

Comment 11

[Joey Armstrong [:joey]]

Comment on attachment 648886 [details] [diff] [review]
support make package QUIET=1

Replaced *_FLAGS use with $(QUIET).
Specify override so QUIET=1 can be used from the command line.
Removed extra flag massaging/assignments mentioned earlier.
a try job is running for the latest patch.

Comment 12

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=b389e7fba51c

Comment 13

[Joey Armstrong [:joey]]

committed changeset 101553:f5ae5fd9142f

Comment 14

[Joey Armstrong [:joey]]

Attachment: support make package QUIET=1

Comment 15

[Joey Armstrong [:joey]]

Comment on attachment 654349 [details] [diff] [review]
support make package QUIET=1

thought this was pushed to inbound already but evidently not.
r=glandium carried forward
re-base, re-try will re-inbound after status from the try job.