Skip to content

Improve --help performance for x.py #48569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 3, 2018
Merged

Improve --help performance for x.py #48569

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffb6291
Improve --help performance for x.py
Phlosioneer Feb 27, 2018
2269ff5
Remove print_what_bootstrap_means
Phlosioneer Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/bootstrap/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ def set_dev_environment(self):
self._download_url = 'https://dev-static.rust-lang.org'


def bootstrap():
def bootstrap(help_triggered):
"""Configure, fetch, build and run the initial bootstrap"""
parser = argparse.ArgumentParser(description='Build rust')
parser.add_argument('--config')
Expand Down Expand Up @@ -708,7 +708,7 @@ def bootstrap():
print(' and so in order to preserve your $HOME this will now')
print(' use vendored sources by default. Note that if this')
print(' does not work you should run a normal build first')
print(' before running a command like `sudo make install`')
print(' before running a command like `sudo ./x.py install`')

if build.use_vendored_sources:
if not os.path.exists('.cargo'):
Expand All @@ -734,7 +734,10 @@ def bootstrap():
if 'dev' in data:
build.set_dev_environment()

build.update_submodules()
# No help text depends on submodules. This check saves ~1 minute of git commands, even if
# all the submodules are present and downloaded!
if not help_triggered:
build.update_submodules()

# Fetch/build the bootstrap
build.build = args.build or build.build_triple()
Expand All @@ -760,7 +763,13 @@ def main():
help_triggered = (
'-h' in sys.argv) or ('--help' in sys.argv) or (len(sys.argv) == 1)
try:
bootstrap()
# If the user is asking for help, let them know that the whole download-and-build
# process has to happen before anything is printed out.
if help_triggered:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this into bootstrap()? Top level main is for catching and handling exceptions only.

print("NOTE: Downloading and compiling bootstrap requirements before processing")
Copy link
Contributor

@petrochenkov petrochenkov Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the word "requirements" used in this meaning? "Downloading requirements" sounds weird.
Maybe "Downloading and building bootstrap and its dependencies ..."?

print(" --help command. See src/bootstrap/README.md for help with common")
print(" commands.")
Copy link
Contributor

@petrochenkov petrochenkov Feb 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get this message only if we actually going to download something or do other long operation.

Actually there's already print_what_bootstrap_means that is supposed to print this exact information, but maybe it doesn't fire for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to print the message every time, because we always rebuild the build tool. So it will never be instant. Also, doing anything more complicated than printing this at the beginning means that we have to go through the whole process to gather information, setup environment variables, and check if anything needs to be done. It would require a lot of logic spread through a lot of the build script, and that complexity simply isn't worth it.

I have never noticed print_what_bootstrap_means. Looking back at it, it only triggers if a download happens, and is immediately followed by a lot of download messages and loading bars. Also, prior to this fix, the very first thing the build script did was update_submodules, so print_what_bootstrap_means would be printed after the most time consuming part of the build script.

The message also doesn't reference --help at all; it states why stuff needs to download before running build commands, but the user will not expect --help to be part of that. My message makes that perfectly clear.

Basically, we either have to print this whenever --help happens, regardless of what we actually need to do; OR we have to wait until the build script sets up enough to know if it needs to download, at which point seconds have already passed and the tool will feel sluggish (asking the user to wait after the user has already waited). And if we want to do the second option, because of how the script is organized, we would either have to do a lot of refactoring, or litter the code with checks and print statements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good.
Could you remove print_what_bootstrap_means (and also self.printed) then?

bootstrap(help_triggered)
if not help_triggered:
print("Build completed successfully in {}".format(
format_build_time(time() - start_time)))
Expand Down