Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 19 additions & 16 deletions src/bootstrap/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def eprint(*args, **kwargs):
print(*args, **kwargs)


def get(base, url, path, checksums, verbose=False):
def get(base, url, path, checksums, verbose=0):
with tempfile.NamedTemporaryFile(delete=False) as temp_file:
temp_path = temp_file.name

Expand All @@ -66,11 +66,11 @@ def get(base, url, path, checksums, verbose=False):
sha256 = checksums[url]
if os.path.exists(path):
if verify(path, sha256, False):
if verbose:
if verbose > 0:
eprint("using already-download file", path)
return
else:
if verbose:
if verbose > 0:
eprint(
"ignoring already-download file",
path,
Expand All @@ -80,12 +80,12 @@ def get(base, url, path, checksums, verbose=False):
download(temp_path, "{}/{}".format(base, url), True, verbose)
if not verify(temp_path, sha256, verbose):
raise RuntimeError("failed verification")
if verbose:
if verbose > 0:
eprint("moving {} to {}".format(temp_path, path))
shutil.move(temp_path, path)
finally:
if os.path.isfile(temp_path):
if verbose:
if verbose > 0:
eprint("removing", temp_path)
os.unlink(temp_path)

Expand Down Expand Up @@ -113,11 +113,11 @@ def _download(path, url, probably_big, verbose, exception):
# If an error occurs:
# - If we are on win32 fallback to powershell
# - Otherwise raise the error if appropriate
if probably_big or verbose:
if probably_big or verbose > 0:
eprint("downloading {}".format(url))

try:
if (probably_big or verbose) and "GITHUB_ACTIONS" not in os.environ:
if (probably_big or verbose > 0) and "GITHUB_ACTIONS" not in os.environ:
option = "--progress-bar"
else:
option = "--silent"
Expand Down Expand Up @@ -180,7 +180,7 @@ def _download(path, url, probably_big, verbose, exception):

def verify(path, expected, verbose):
"""Check if the sha256 sum of the given path is valid"""
if verbose:
if verbose > 0:
eprint("verifying", path)
with open(path, "rb") as source:
found = hashlib.sha256(source.read()).hexdigest()
Expand All @@ -194,7 +194,7 @@ def verify(path, expected, verbose):
return verified


def unpack(tarball, tarball_suffix, dst, verbose=False, match=None):
def unpack(tarball, tarball_suffix, dst, verbose=0, match=None):
"""Unpack the given tarball file"""
eprint("extracting", tarball)
fname = os.path.basename(tarball).replace(tarball_suffix, "")
Expand All @@ -208,7 +208,7 @@ def unpack(tarball, tarball_suffix, dst, verbose=False, match=None):
name = name[len(match) + 1 :]

dst_path = os.path.join(dst, name)
if verbose:
if verbose > 0:
eprint(" extracting", member)
tar.extract(member, dst)
src_path = os.path.join(dst, member)
Expand All @@ -218,9 +218,9 @@ def unpack(tarball, tarball_suffix, dst, verbose=False, match=None):
shutil.rmtree(os.path.join(dst, fname))


def run(args, verbose=False, exception=False, is_bootstrap=False, **kwargs):
def run(args, verbose=0, exception=False, is_bootstrap=False, **kwargs):
"""Run a child program in a new process"""
if verbose:
if verbose > 0:
eprint("running: " + " ".join(args))
sys.stdout.flush()
# Ensure that the .exe is used on Windows just in case a Linux ELF has been
Expand All @@ -233,7 +233,7 @@ def run(args, verbose=False, exception=False, is_bootstrap=False, **kwargs):
code = ret.wait()
if code != 0:
err = "failed to run: " + " ".join(args)
if verbose or exception:
if verbose > 0 or exception:
raise RuntimeError(err)
# For most failures, we definitely do want to print this error, or the user will have no
# idea what went wrong. But when we've successfully built bootstrap and it failed, it will
Expand Down Expand Up @@ -293,13 +293,13 @@ def default_build_triple(verbose):
version = version.decode(default_encoding)
host = next(x for x in version.split("\n") if x.startswith("host: "))
triple = host.split("host: ")[1]
if verbose:
if verbose > 0:
eprint(
"detected default triple {} from pre-installed rustc".format(triple)
)
return triple
except Exception as e:
if verbose:
if verbose > 0:
eprint("pre-installed rustc not detected: {}".format(e))
eprint("falling back to auto-detect")

Expand Down Expand Up @@ -699,7 +699,7 @@ def download_toolchain(self):
# Unpack the tarballs in parallel.
# In Python 2.7, Pool cannot be used as a context manager.
pool_size = min(len(tarballs_download_info), get_cpus())
if self.verbose:
if self.verbose > 0:
print(
"Choosing a pool size of",
pool_size,
Expand Down Expand Up @@ -1126,6 +1126,8 @@ def build_bootstrap_cmd(self, env):
]
# verbose cargo output is very noisy, so only enable it with -vv
args.extend("--verbose" for _ in range(self.verbose - 1))
if self.verbose < 0:
args.append("--quiet")
Comment on lines 1127 to +1130
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question [MUTEX 1/2]: hm, this combination is somewhat interesting. Does cargo support --quiet --verbose?

EDIT: no, you cannot:

$ cargo check --quiet --verbose
error: cannot set both --verbose and --quiet

Can we also:

  • Either do an exclusivity check here, or
  • Reject the combination of both --verbose/-vv et al. with --quiet during arg parsing?

I kinda don't want to have to wait until we reach cargo to know that this is not a valid combo.


target_features = []
if self.get_toml("crt-static", build_section) == "true":
Expand Down Expand Up @@ -1276,6 +1278,7 @@ def parse_args(args):
"--warnings", choices=["deny", "warn", "default"], default="default"
)
parser.add_argument("-v", "--verbose", action="count", default=0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion [MUTEX 2/2]:

Looks like Python's argparse allows us to use "mutually-exclusive subgroups" (see https://docs.python.org/3/library/argparse.html#mutual-exclusion), I would suggest making --verbose and --quiet mutually exclusive for the reasons described in [MUTEX 1/2].

parser.add_argument("-q", "--quiet", action="store_const", const=-1, dest="verbose")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Discussion: hm, I thought about this a bit, I was wondering why this was using store_const and not store_{true,false}, and it's because this is the minimal change to repurpose verbose args/fields in bootstrap.py. Otherwise, you'd need to change all the if self.verbose: and friends to also do if self.verbose and not self.quiet etc.

I'm slightly conflicted, because now "verbose" is more like

                suppress output       increasing verbosity
                ◄───────────────── ─────────────────────────►  
                quiet             0             -v        -vv  ...
verbose :=      -1                0              1         2   ...

but then again, it's not really the end of the world. We may want to leave a comment here re. the intention / recycling of verbose tho.


return parser.parse_known_args(args)[0]

Expand Down
11 changes: 11 additions & 0 deletions src/bootstrap/src/core/build_steps/llvm.rs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Question: the general idea of a --quiet flag seems reasonable to me. Can you say more on your use case? I want to learn more re. what you intend to be using --quiet for. Is it just to suppress some of the "non-critical messages" for local dev?

I ask because some of the warnings can indicate real, genuine problems.

Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,17 @@ fn configure_cmake(
// LLVM and LLD builds can produce a lot of those and hit CI limits on log size.
cfg.define("CMAKE_INSTALL_MESSAGE", "LAZY");

if builder.config.quiet {
// Only log errors and warnings from `cmake`.
cfg.define("CMAKE_MESSAGE_LOG_LEVEL", "WARNING");
// Don't log output from `ninja` or `make`.
if builder.ninja() {
cfg.build_arg("--quiet");
} else {
cfg.build_arg("-s");
}
}

// Do not allow the user's value of DESTDIR to influence where
// LLVM will install itself. LLVM must always be installed in our
// own build directories.
Expand Down
5 changes: 5 additions & 0 deletions src/bootstrap/src/core/builder/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,11 @@ impl Builder<'_> {
}
};

// Optionally suppress cargo output.
if self.config.quiet {
cargo.arg("--quiet");
}

// Run cargo from the source root so it can find .cargo/config.
// This matters when using vendoring and the working directory is outside the repository.
cargo.current_dir(&self.src);
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/src/core/config/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ pub struct Config {
pub config: Option<PathBuf>,
pub jobs: Option<u32>,
pub cmd: Subcommand,
pub quiet: bool,
pub incremental: bool,
pub dump_bootstrap_shims: bool,
/// Arguments appearing after `--` to be forwarded to tools,
Expand Down Expand Up @@ -368,6 +369,7 @@ impl Config {
let Flags {
cmd: flags_cmd,
verbose: flags_verbose,
quiet: flags_quiet,
incremental: flags_incremental,
config: flags_config,
build_dir: flags_build_dir,
Expand Down Expand Up @@ -1432,6 +1434,7 @@ impl Config {
print_step_timings: build_print_step_timings.unwrap_or(false),
profiler: build_profiler.unwrap_or(false),
python: build_python.map(PathBuf::from),
quiet: flags_quiet,
reproducible_artifacts: flags_reproducible_artifact,
reuse: build_reuse.map(PathBuf::from),
rust_analyzer_info,
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/src/core/config/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ pub struct Flags {
/// use verbose output (-vv for very verbose)
pub verbose: u8, // each extra -v after the first is passed to Cargo
#[arg(global = true, short, long)]
/// use quiet output
pub quiet: bool,
#[arg(global = true, short, long)]
/// use incremental compilation
pub incremental: bool,
#[arg(global = true, long, value_hint = clap::ValueHint::FilePath, value_name = "FILE")]
Expand Down
Loading
Loading