1411462 - Unbust configure --help

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

`configure --help` stopped working after 370a4a2a7898 (bug 1405982).

I already have a patch authored...

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1411462 - Normalize when argument earlier in imply_option();

It stopped working due to 370a4a2a7898 (bug 1405982).

Review commit: https://reviewboard.mozilla.org/r/192704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192704/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1411462 - Add test for `configure --help`;

We add a simple test that `configure --help` works.

We do this via a cram test. I added the tests to build/tests because
I'm not sure where else they should go.

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
3) It is printed twice (!!)

I reckon these can be follow-up improvements.

Review commit: https://reviewboard.mozilla.org/r/192706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192706/

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Mike Hommey [:glandium]]

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

[Gregory Szorc [:gps]]

Attachment: Bug 1411462 - Abort when I/O error seen;

Before, I/O errors writing to stdout/stderr (e.g. due to broken pipe)
would result in handleError() being called and execution would
keep running. This would potentially result in an error message for
every log/line failure being printed to stderr.

We change the behavior so I/O failures are fatal and abort
execution.

We test the new behavior by changing a test to pipe to `head`
directly. Since `head` exits once it has seen sufficient output,
this results in an EPIPE which now results in immediate program
termination.

Review commit: https://reviewboard.mozilla.org/r/197354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197354/

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8921695 [details]
Bug 1411462 - Normalize when argument earlier in imply_option();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192704/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8921696 [details]
Bug 1411462 - Add test for `configure --help`;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192706/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8921695 [details]
Bug 1411462 - Normalize when argument earlier in imply_option();

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

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8921695 [details]
Bug 1411462 - Normalize when argument earlier in imply_option();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192704/diff/2-3/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8921696 [details]
Bug 1411462 - Add test for `configure --help`;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192706/diff/2-3/

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8926122 [details]
Bug 1411462 - Abort when I/O error seen;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197354/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e6769924323
https://hg.mozilla.org/mozilla-central/rev/6fff2c7ad3fc
https://hg.mozilla.org/mozilla-central/rev/a4f05c8137ab