Closed Bug 1151834 Opened 5 years ago Closed 5 years ago

(MozBootstrap) Allow users to bootstrap without any interactive prompts.

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mrrrgn, Assigned: mrrrgn)

References

Details

Attachments

(1 file, 3 obsolete files)

Right now users are given an interactive prompt, to choose between bootstrapping a system for FF Desktop or FF Android. This makes it more difficult to use bootstrap.py in an automated fashion. To remedy this, users should also have the option to pass in their choice as a script argument.
Assignee: nobody → winter2718
Blocks: 1151508
Summary: (MozBootstrap) Allow users to choose a bootstrap environment without an interactive prompt. → (MozBootstrap) Allow users to choose a bootstrap application without an interactive prompt.
Attached patch choices-1-of-2 (obsolete) — Splinter Review
After this patch, it will also be necessary to allow users to run apt-get with the '-y' option.
Attachment #8589108 - Attachment description: choices → choices-1-of-2
Attached patch choices-2-of-2 (obsolete) — Splinter Review
Attachment #8589130 - Flags: review?(gps)
Attachment #8589108 - Flags: review?(gps)
Summary: (MozBootstrap) Allow users to choose a bootstrap application without an interactive prompt. → (MozBootstrap) Allow users to bootstrap with any interactive prompts.
Summary: (MozBootstrap) Allow users to bootstrap with any interactive prompts. → (MozBootstrap) Allow users to bootstrap without any interactive prompts.
Should this be under Release Engineering?
Component: Developer Tools → Tools
Product: Developer Documentation → Release Engineering
QA Contact: hwine
Release Engineering works: it's a little tricky, just because I had thought of bootstrap as being a developer tool first and foremost.
*ping*
This should be tracked as part of the Firefox build system.

Sorry for not reviewing - you hit me up for review just as I was about to go to Montreal for PyCon and a Mercurial Sprint. I'm back now. Will make my way through email and review backlog today.
Component: Tools → Build Config
Product: Release Engineering → Core
QA Contact: hwine
Attachment #8589108 - Attachment is patch: true
Comment on attachment 8589108 [details] [diff] [review]
choices-1-of-2

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

This patch has CRLF characters. Please normalize line endings to LF before committing. Please change your editor on Windows to save LF endings.

r+ on the feature. r- on the implementation. So f+.

::: python/mozboot/bin/bootstrap.py
@@ +131,5 @@
> +                      help='The type of the repository. This defines how we fetch file '
> +                      'content. Like --repo, you should not need to set this.')
> +    parser.add_option('--application-choice', dest='application_choice', type=int,
> +                      help='Pass in an application choice (instead of using the '
> +                      'default interactive prompt)')

Passing integers as CLI arguments is opaque and not very use friendly. Please rewrite this so the CLI takes strings like "desktop" and "android" for the argument.
Attachment #8589108 - Flags: review?(gps) → feedback+
Comment on attachment 8589130 [details] [diff] [review]
choices-2-of-2

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

This is fine except for the bikeshed. Next review should be a pretty straightforward rubber stamp r+.

::: python/mozboot/bin/bootstrap.py
@@ +136,5 @@
>      parser.add_option('--application-choice', dest='application_choice', type=int,
>                        help='Pass in an application choice (instead of using the '
>                        'default interactive prompt)')
> +    parser.add_option('--assume-yes', dest='assume_yes', action='store_true',
> +                      help='Answer yes to any (Y/n) interactive prompts')

Let's make this --no-interactive, since that's what the feature does.
Attachment #8589130 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 8589108 [details] [diff] [review]
> choices-1-of-2
> 
> Review of attachment 8589108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch has CRLF characters. Please normalize line endings to LF before
> committing. Please change your editor on Windows to save LF endings.
> 
> r+ on the feature. r- on the implementation. So f+.
> 
> ::: python/mozboot/bin/bootstrap.py
> @@ +131,5 @@
> > +                      help='The type of the repository. This defines how we fetch file '
> > +                      'content. Like --repo, you should not need to set this.')
> > +    parser.add_option('--application-choice', dest='application_choice', type=int,
> > +                      help='Pass in an application choice (instead of using the '
> > +                      'default interactive prompt)')
> 
> Passing integers as CLI arguments is opaque and not very use friendly.
> Please rewrite this so the CLI takes strings like "desktop" and "android"
> for the argument.

Oddly, I don't use Windows. Not sure how the strange endings got in there. I'll make sure they are normalized.
Attached patch mozboot.total.diff (obsolete) — Splinter Review
Attachment #8589108 - Attachment is obsolete: true
Attachment #8589130 - Attachment is obsolete: true
Attachment #8594607 - Flags: review?(gps)
Comment on attachment 8594607 [details] [diff] [review]
mozboot.total.diff

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

Almost...

::: python/mozboot/mozboot/base.py
@@ +156,5 @@
>          print('Executing as root:', subprocess.list2cmdline(command))
>          subprocess.check_call(command, stdin=sys.stdin)
>      def yum_install(self, *packages):
>          command = ['yum', 'install']
> +        if self.no_interactive is True:

"is True" is considered an anti-pattern in Firefox since "if foo" almost always does the same thing. Please nix this here and throughout the patch.

::: python/mozboot/mozboot/bootstrap.py
@@ +5,5 @@
>  from __future__ import print_function
>  import platform
>  import sys
>  import os.path
> +import collections

Sadly, collections was introduced in Python 2.7 and this file must be able to run on Python 2.6.

That being said, we may want to audit and see what is actually holding up back to Python 2.6. It was originally OS X 10.6. I can make an argument we can drop 10.6 as a *development* platform. But that's for a follow-up bug.
Attachment #8594607 - Flags: review?(gps) → feedback+
Attachment #8594607 - Flags: checkin+
Attachment #8594607 - Attachment is obsolete: true
Attachment #8596054 - Flags: checkin+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8596054 - Flags: checkin+
Comment on attachment 8596054 [details] [diff] [review]
mozboot.total.diff

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

LGTM!
Attachment #8596054 - Flags: review+
Attachment #8596054 - Flags: checkin?(gps)
Attachment #8596054 - Flags: checkin?(gps) → checkin+
https://hg.mozilla.org/mozilla-central/rev/5da7e0d7f32e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.