Closed Bug 1411462 Opened 7 years ago Closed 7 years ago

Unbust configure --help

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(3 files)

Bug 1411462 - Normalize when argument earlier in imply_option();
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1411462 - Add test for `configure --help`;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1411462 - Abort when I/O error seen;
59 bytes, text/x-review-board-request
glandium
: review+
Details 
`configure --help` stopped working after 370a4a2a7898 (bug 1405982).

I already have a patch authored...
Comment on attachment 8921695 [details]
Bug 1411462 - Normalize when argument earlier in imply_option();

https://reviewboard.mozilla.org/r/192704/#review197940

As per bug 1411081 comment 19, this should be fixed by making the uses from "when" count.
Attachment #8921695 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8921696 [details]
Bug 1411462 - Add test for `configure --help`;

https://reviewboard.mozilla.org/r/192706/#review197942

::: commit-message-8d758:10
(Diff revision 1)
> +1) piping `./configure --help` to `head` directly causes a Python
> +   traceback (presumably due to the pipe disappearing once N lines
> +   have been read)

Note, this sounds like something that should be fixed.

::: commit-message-8d758:13
(Diff revision 1)
> +This test uncovers a few interesting things:
> +
> +1) piping `./configure --help` to `head` directly causes a Python
> +   traceback (presumably due to the pipe disappearing once N lines
> +   have been read)
> +2) "checking for vcs source checkout" is printed for --help

No surprise there, since there's a dependency on '--help' for vcs_checkout_type. That, in turn, is because it's used as a dependency of a when in check_prog, which passes that when to an option.

I'd argue we don't need that @checking on vcs_checkout_type.

::: commit-message-8d758:14
(Diff revision 1)
> +
> +1) piping `./configure --help` to `head` directly causes a Python
> +   traceback (presumably due to the pipe disappearing once N lines
> +   have been read)
> +2) "checking for vcs source checkout" is printed for --help
> +3) It is printed twice (!!)

/That/ is more interesting. That's due to memoization on _value_for_depends, which is called twice for the object corresponding to vcs_source_checkout, one with need_help_dependency=True, and one with need_help_dependency=False.

A way to fix this would be to do a manual memoization, ignoring the value of need_help_dependency, although, when running in the linter, we do care about not using the cache from need_help_dependency=False when passed need_help_dependency=True.

Also, there's recursively too much memoization.
Attachment #8921696 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #3)
> Comment on attachment 8921695 [details]
> Bug 1411462 - Make ./configure --help work again;
> 
> https://reviewboard.mozilla.org/r/192704/#review197940
> 
> As per bug 1411081 comment 19, this should be fixed by making the uses from
> "when" count.

This fixes it:

diff --git a/python/mozbuild/mozbuild/configure/__init__.py b/python/mozbuild/mozbuild/configure/__init__.py
index 09ea1d068803a..f0ad8f127ac10 100644
--- a/python/mozbuild/mozbuild/configure/__init__.py
+++ b/python/mozbuild/mozbuild/configure/__init__.py
@@ -903,16 +903,18 @@ class ConfigureSandbox(dict):
 
         The `value` argument can also be (and usually is) a reference to a
         @depends function, in which case the result of that function will be
         used as per the descripted mapping above.
 
         The `reason` argument indicates what caused the option to be implied.
         It is necessary when it cannot be inferred from the `value`.
         '''
+        when = self._normalize_when(when, 'imply_option')
+
         # Don't do anything when --help was on the command line
         if self._help:
             return
         if not reason and isinstance(value, SandboxDependsFunction):
             deps = self._depends[value].dependencies
             possible_reasons = [d for d in deps if d != self._help_option]
             if len(possible_reasons) == 1:
                 if isinstance(possible_reasons[0], Option):
@@ -925,18 +927,16 @@ class ConfigureSandbox(dict):
             reason = "imply_option at %s:%s" % (filename, line)
 
         if not reason:
             raise ConfigureError(
                 "Cannot infer what implies '%s'. Please add a `reason` to "
                 "the `imply_option` call."
                 % option)
 
-        when = self._normalize_when(when, 'imply_option')
-
         prefix, name, values = Option.split_option(option)
         if values != ():
             raise ConfigureError("Implied option must not contain an '='")
 
         self._implied_options.append(ReadOnlyNamespace(
             option=option,
             prefix=prefix,
             name=name,
Comment on attachment 8921695 [details]
Bug 1411462 - Normalize when argument earlier in imply_option();

https://reviewboard.mozilla.org/r/192704/#review202738
Attachment #8921695 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8926122 [details]
Bug 1411462 - Abort when I/O error seen;

https://reviewboard.mozilla.org/r/197354/#review202740

::: python/mozbuild/mozbuild/configure/util.py:152
(Diff revision 1)
>                      self._stdout.flush()
>                  stream = self._stderr
>                  msg = '%s\n' % self.format(record)
>              stream.write(msg)
>              stream.flush()
>          except (KeyboardInterrupt, SystemExit):

You could add IOError here, since you're just re-raising it.
Attachment #8926122 - Flags: review?(mh+mozilla) → review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e6769924323
Normalize when argument earlier in imply_option(); r=glandium
https://hg.mozilla.org/integration/autoland/rev/6fff2c7ad3fc
Add test for `configure --help`; r=glandium
https://hg.mozilla.org/integration/autoland/rev/a4f05c8137ab
Abort when I/O error seen; r=glandium
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: