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

RESOLVED WONTFIX

Status

()

Core
Build Config
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
+1000
(Assignee)

Updated

6 years ago
Assignee: nobody → joey
(Assignee)

Comment 2

6 years ago
Created attachment 647745 [details] [diff] [review]
support make package QUIET=1
(Assignee)

Comment 3

6 years ago
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.
Attachment #647745 - Flags: review?(mh+mozilla)
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.
Attachment #647745 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 5

6 years ago
(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

6 years ago
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.
(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).
(Assignee)

Comment 8

6 years ago
> > 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.
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.
(Assignee)

Comment 10

6 years ago
Created attachment 648886 [details] [diff] [review]
support make package QUIET=1
(Assignee)

Updated

6 years ago
Attachment #647745 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
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.
Attachment #648886 - Flags: review?(mh+mozilla)
Attachment #648886 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 13

6 years ago
committed changeset 101553:f5ae5fd9142f
(Assignee)

Comment 14

6 years ago
Created attachment 654349 [details] [diff] [review]
support make package QUIET=1
(Assignee)

Updated

6 years ago
Attachment #648886 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.