From 0f522c2bc9f18449db67c8783f94360f1a6e1ab8 Mon Sep 17 00:00:00 2001 From: abylaw Date: Fri, 21 Feb 2025 02:08:08 -0800 Subject: [PATCH 1/2] Remove deserializing/reserializing toml from configure.py --- src/bootstrap/bootstrap_test.py | 73 ++++++-- src/bootstrap/configure.py | 313 +++++++++++++++----------------- 2 files changed, 202 insertions(+), 184 deletions(-) diff --git a/src/bootstrap/bootstrap_test.py b/src/bootstrap/bootstrap_test.py index 7494536539d7b..b608dfb5168a7 100644 --- a/src/bootstrap/bootstrap_test.py +++ b/src/bootstrap/bootstrap_test.py @@ -26,9 +26,9 @@ def serialize_and_parse(configure_args, bootstrap_args=None): if bootstrap_args is None: bootstrap_args = bootstrap.FakeArgs() - section_order, sections, targets = configure.parse_args(configure_args) + config = configure.parse_args(configure_args) buffer = StringIO() - configure.write_config_toml(buffer, section_order, targets, sections) + configure.write_config_toml(buffer, config) build = bootstrap.RustBuild(config_toml=buffer.getvalue(), args=bootstrap_args) try: @@ -108,50 +108,69 @@ def test_same_dates(self): ) -class ParseArgsInConfigure(unittest.TestCase): - """Test if `parse_args` function in `configure.py` works properly""" +class ValidateArgsInConfigure(unittest.TestCase): + """Test if `validate_args` function in `configure.py` works properly""" @patch("configure.err") def test_unknown_args(self, err): # It should be print an error message if the argument doesn't start with '--' - configure.parse_args(["enable-full-tools"]) + configure.validate_args(["enable-full-tools"]) err.assert_called_with("Option 'enable-full-tools' is not recognized") err.reset_mock() # It should be print an error message if the argument is not recognized - configure.parse_args(["--some-random-flag"]) + configure.validate_args(["--some-random-flag"]) err.assert_called_with("Option '--some-random-flag' is not recognized") @patch("configure.err") def test_need_value_args(self, err): """It should print an error message if a required argument value is missing""" - configure.parse_args(["--target"]) + configure.validate_args(["--target"]) err.assert_called_with("Option '--target' needs a value (--target=val)") + @patch("configure.err") + def test_duplicate_args(self, err): + """It should print an error message if an option is passed more than once""" + configure.validate_args(["--enable-sccache", "--enable-sccache"]) + err.assert_called_with("Option 'sccache' provided more than once") + err.reset_mock() + + configure.validate_args(["--enable-sccache", "--disable-sccache"]) + err.assert_called_with("Option 'sccache' provided more than once") + @patch("configure.err") def test_option_checking(self, err): # Options should be checked even if `--enable-option-checking` is not passed - configure.parse_args(["--target"]) + configure.validate_args(["--target"]) err.assert_called_with("Option '--target' needs a value (--target=val)") err.reset_mock() # Options should be checked if `--enable-option-checking` is passed - configure.parse_args(["--enable-option-checking", "--target"]) + configure.validate_args(["--enable-option-checking", "--target"]) err.assert_called_with("Option '--target' needs a value (--target=val)") err.reset_mock() # Options should not be checked if `--disable-option-checking` is passed - configure.parse_args(["--disable-option-checking", "--target"]) + configure.validate_args(["--disable-option-checking", "--target"]) err.assert_not_called() - @patch("configure.parse_example_config", lambda known_args, _: known_args) + @patch("configure.p") + def test_warns_on_rpm_args(self, p): + """It should print a warning if rpm args are passed, and ignore them""" + configure.validate_args(["--infodir=/usr/share/info"]) + p.assert_called_with("WARNING: infodir will be ignored") + p.reset_mock() + + configure.validate_args(["--localstatedir=/var/lib/info"]) + p.assert_called_with("WARNING: localstatedir will be ignored") + def test_known_args(self): # It should contain known and correct arguments - known_args = configure.parse_args(["--enable-full-tools"]) + known_args = configure.validate_args(["--enable-full-tools"]) self.assertTrue(known_args["full-tools"][0][1]) - known_args = configure.parse_args(["--disable-full-tools"]) + known_args = configure.validate_args(["--disable-full-tools"]) self.assertFalse(known_args["full-tools"][0][1]) # It should contain known arguments and their values - known_args = configure.parse_args(["--target=x86_64-unknown-linux-gnu"]) + known_args = configure.validate_args(["--target=x86_64-unknown-linux-gnu"]) self.assertEqual(known_args["target"][0][1], "x86_64-unknown-linux-gnu") - known_args = configure.parse_args(["--target", "x86_64-unknown-linux-gnu"]) + known_args = configure.validate_args(["--target", "x86_64-unknown-linux-gnu"]) self.assertEqual(known_args["target"][0][1], "x86_64-unknown-linux-gnu") @@ -173,6 +192,30 @@ def test_set_target(self): build.get_toml("cc", section="target.x86_64-unknown-linux-gnu"), "gcc" ) + def test_set_target_option(self): + build = serialize_and_parse(["--build=x86_64-unknown-linux-gnu", "--llvm-config=/usr/bin/llvm-config"]) + self.assertEqual( + build.get_toml("llvm-config", section="target.x86_64-unknown-linux-gnu"), "/usr/bin/llvm-config" + ) + + def test_set_targets(self): + # Multiple targets can be set + build = serialize_and_parse(["--set", "target.x86_64-unknown-linux-gnu.cc=gcc", "--set", "target.aarch64-apple-darwin.cc=clang"]) + self.assertEqual( + build.get_toml("cc", section="target.x86_64-unknown-linux-gnu"), "gcc" + ) + self.assertEqual( + build.get_toml("cc", section="target.aarch64-apple-darwin"), "clang" + ) + + build = serialize_and_parse(["--target", "x86_64-unknown-linux-gnu,aarch64-apple-darwin"]) + self.assertNotEqual( + build.config_toml.find("target.aarch64-apple-darwin"), -1 + ) + self.assertNotEqual( + build.config_toml.find("target.x86_64-unknown-linux-gnu"), -1 + ) + def test_set_top_level(self): build = serialize_and_parse(["--set", "profile=compiler"]) self.assertEqual(build.get_toml("profile"), "compiler") diff --git a/src/bootstrap/configure.py b/src/bootstrap/configure.py index ac971a64d7c22..4df9f9b09a2df 100755 --- a/src/bootstrap/configure.py +++ b/src/bootstrap/configure.py @@ -374,12 +374,22 @@ def is_value_list(key): VERBOSE = False - -# Parse all command line arguments into one of these three lists, handling -# boolean and value-based options separately +# Parse command line arguments into a valid build configuration. def parse_args(args): + known_args = validate_args(args) + config = generate_config(known_args) + if "profile" not in config: + _set("profile", "dist", config) + _set("build.configure-args", args, config) + return config + +# Validate command line arguments, throwing an error if there are any unknown +# arguments, missing values or duplicate arguments when option-checking is also +# passed as an argument. Returns a dictionary of known arguments. +def validate_args(args): unknown_args = [] need_value_args = [] + duplicate_args = [] known_args = {} i = 0 @@ -398,6 +408,10 @@ def parse_args(args): key = keyval[0] if option.name != key: continue + if option.name == "infodir" or option.name == "localstatedir": + # These are used by rpm, but aren't accepted by x.py. + # Give a warning that they're ignored, but not a hard error. + p("WARNING: {} will be ignored".format(option.name)) if len(keyval) > 1: value = keyval[1] @@ -414,10 +428,13 @@ def parse_args(args): value = False else: continue - found = True + if option.name not in known_args: known_args[option.name] = [] + elif option.name in known_args and option.name != "set": + duplicate_args.append(option.name) + known_args[option.name].append((option, value)) break @@ -435,24 +452,17 @@ def parse_args(args): err("Option '" + unknown_args[0] + "' is not recognized") if len(need_value_args) > 0: err("Option '{0}' needs a value ({0}=val)".format(need_value_args[0])) + for key, values in known_args.items(): + if len(values) > 1 and key != "set": + err("Option '{}' provided more than once".format(key)) global VERBOSE VERBOSE = "verbose-configure" in known_args - config = {} - - set("build.configure-args", args, config) - apply_args(known_args, option_checking, config) - return parse_example_config(known_args, config) - - -def build(known_args): - if "build" in known_args: - return known_args["build"][-1][1] - return bootstrap.default_build_triple(verbose=False) + return known_args -def set(key, value, config): +def _set(key, value, config): if isinstance(value, list): # Remove empty values, which value.split(',') tends to generate and # replace single quotes for double quotes to ensure correct parsing. @@ -483,7 +493,9 @@ def set(key, value, config): arr = arr[part] -def apply_args(known_args, option_checking, config): +# Convert a validated list of command line arguments into configuration +def generate_config(known_args): + config = {} for key in known_args: # The `set` option is special and can be passed a bunch of times if key == "set": @@ -495,132 +507,168 @@ def apply_args(known_args, option_checking, config): value = False else: value = keyval[1] - set(keyval[0], value, config) + _set(keyval[0], value, config) continue - # Ensure each option is only passed once arr = known_args[key] - if option_checking and len(arr) > 1: - err("Option '{}' provided more than once".format(key)) option, value = arr[-1] # If we have a clear avenue to set our value in rustbuild, do so if option.rustbuild is not None: - set(option.rustbuild, value, config) + _set(option.rustbuild, value, config) continue # Otherwise we're a "special" option and need some extra handling, so do # that here. - build_triple = build(known_args) + if "build" in known_args: + build_triple = known_args["build"][-1][1] + else: + build_triple = bootstrap.default_build_triple(verbose=False) if option.name == "sccache": - set("build.ccache", "sccache", config) + _set("build.ccache", "sccache", config) elif option.name == "local-rust": for path in os.environ["PATH"].split(os.pathsep): if os.path.exists(path + "/rustc"): - set("build.rustc", path + "/rustc", config) + _set("build.rustc", path + "/rustc", config) break for path in os.environ["PATH"].split(os.pathsep): if os.path.exists(path + "/cargo"): - set("build.cargo", path + "/cargo", config) + _set("build.cargo", path + "/cargo", config) break elif option.name == "local-rust-root": - set("build.rustc", value + "/bin/rustc", config) - set("build.cargo", value + "/bin/cargo", config) + _set("build.rustc", value + "/bin/rustc", config) + _set("build.cargo", value + "/bin/cargo", config) elif option.name == "llvm-root": - set( + _set( "target.{}.llvm-config".format(build_triple), value + "/bin/llvm-config", config, ) elif option.name == "llvm-config": - set("target.{}.llvm-config".format(build_triple), value, config) + _set("target.{}.llvm-config".format(build_triple), value, config) elif option.name == "llvm-filecheck": - set("target.{}.llvm-filecheck".format(build_triple), value, config) + _set("target.{}.llvm-filecheck".format(build_triple), value, config) elif option.name == "tools": - set("build.tools", value.split(","), config) + _set("build.tools", value.split(","), config) elif option.name == "bootstrap-cache-path": - set("build.bootstrap-cache-path", value, config) + _set("build.bootstrap-cache-path", value, config) elif option.name == "codegen-backends": - set("rust.codegen-backends", value.split(","), config) + _set("rust.codegen-backends", value.split(","), config) elif option.name == "host": - set("build.host", value.split(","), config) + _set("build.host", value.split(","), config) elif option.name == "target": - set("build.target", value.split(","), config) + _set("build.target", value.split(","), config) elif option.name == "full-tools": - set("rust.codegen-backends", ["llvm"], config) - set("rust.lld", True, config) - set("rust.llvm-tools", True, config) - set("rust.llvm-bitcode-linker", True, config) - set("build.extended", True, config) + _set("rust.codegen-backends", ["llvm"], config) + _set("rust.lld", True, config) + _set("rust.llvm-tools", True, config) + _set("rust.llvm-bitcode-linker", True, config) + _set("build.extended", True, config) elif option.name in ["option-checking", "verbose-configure"]: # this was handled above pass elif option.name == "dist-compression-formats": - set("dist.compression-formats", value.split(","), config) + _set("dist.compression-formats", value.split(","), config) else: raise RuntimeError("unhandled option {}".format(option.name)) + return config -# "Parse" the `config.example.toml` file into the various sections, and we'll -# use this as a template of a `config.toml` to write out which preserves -# all the various comments and whatnot. -# -# Note that the `target` section is handled separately as we'll duplicate it -# per configured target, so there's a bit of special handling for that here. -def parse_example_config(known_args, config): - sections = {} - cur_section = None - sections[None] = [] - section_order = [None] - targets = {} - top_level_keys = [] - - with open(rust_dir + "/config.example.toml") as example_config: - example_lines = example_config.read().split("\n") - for line in example_lines: - if cur_section is None: - if line.count("=") == 1: - top_level_key = line.split("=")[0] - top_level_key = top_level_key.strip(" #") - top_level_keys.append(top_level_key) - if line.startswith("["): - cur_section = line[1:-1] - if cur_section.startswith("target"): - cur_section = "target" - elif "." in cur_section: - raise RuntimeError( - "don't know how to deal with section: {}".format(cur_section) - ) - sections[cur_section] = [line] - section_order.append(cur_section) - else: - sections[cur_section].append(line) +def get_configured_targets(config): + targets = set() + if "build" in config and "build" in config["build"]: + targets.add(config["build"]["build"]) + else: + targets.add(bootstrap.default_build_triple(verbose=False)) - # Fill out the `targets` array by giving all configured targets a copy of the - # `target` section we just loaded from the example config - configured_targets = [build(known_args)] if "build" in config: if "host" in config["build"]: - configured_targets += config["build"]["host"] + targets.update(config["build"]["host"]) if "target" in config["build"]: - configured_targets += config["build"]["target"] + targets.update(config["build"]["target"]) if "target" in config: for target in config["target"]: - configured_targets.append(target) - for target in configured_targets: - targets[target] = sections["target"][:] - # For `.` to be valid TOML, it needs to be quoted. But `bootstrap.py` doesn't use a proper TOML parser and fails to parse the target. - # Avoid using quotes unless it's necessary. - targets[target][0] = targets[target][0].replace( - "x86_64-unknown-linux-gnu", - "'{}'".format(target) if "." in target else target, - ) + targets.add(target) + return list(targets) - if "profile" not in config: - set("profile", "dist", config) - configure_file(sections, top_level_keys, targets, config) - return section_order, sections, targets + +def write_block(f, config, block): + last_line = block[-1] + key = last_line.split("=")[0].strip(' #') + value = config[key] if key in config else None + if value is not None: + for ln in block[:-1]: + f.write(ln + "\n") + f.write("{} = {}\n".format(key, to_toml(value))) + + +def write_section(f, config, section_lines): + block = [] + for line in section_lines[1:]: + if line.count("=") == 1: + block.append(line) + write_block(f, config, block) + block = [] + else: + block.append(line) + + +# Write out the configuration toml file to f, using config.example.toml as a +# template. +def write_config_toml(f, config): + with open(rust_dir + "/config.example.toml") as example_config: + lines = example_config.read().split("\n") + + section_name = None + section = [] + block = [] + + # Drop the initial comment block + for i, line in enumerate(lines): + if not line.startswith("#"): + break + + for line in lines[i:]: + if line.startswith("["): + if section: + # Write out the previous section before starting a new one. + # + # Note that the `target` section is handled separately as we'll + # duplicate it per configured target, so there's a bit of special + # handling for that here. + if section_name.startswith("target"): + for target in get_configured_targets(config): + # For `.` to be valid TOML, it needs to be quoted. But `bootstrap.py` doesn't + # use a proper TOML parser and fails to parse the target. + # Avoid using quotes unless it's necessary. + target_trip = "'{}'".format(target) if "." in target else target + f.write("\n[target.{}]\n".format(target_trip)) + if "target" in config and target in config["target"]: + write_section(f, config["target"][target], section) + elif "." in section_name: + raise RuntimeError( + "don't know how to deal with section: {}".format(section_name) + ) + else: + f.write("\n[{}]\n".format(section_name)) + if section_name in config: + write_section(f, config[section_name], section) + # Start a new section + section = [] + section_name = None + section_name = line[1:-1] + section.append(line) + elif section_name is not None: + section.append(line) + else: + # this a top-level configuration + if line.count("=") == 1: + block.append(line) + write_block(f, config, block) + block = [] + else: + block.append(line) def is_number(value): @@ -664,79 +712,6 @@ def to_toml(value): raise RuntimeError("no toml") -def configure_section(lines, config): - for key in config: - value = config[key] - found = False - for i, line in enumerate(lines): - if not line.startswith("#" + key + " = "): - continue - found = True - lines[i] = "{} = {}".format(key, to_toml(value)) - break - if not found: - # These are used by rpm, but aren't accepted by x.py. - # Give a warning that they're ignored, but not a hard error. - if key in ["infodir", "localstatedir"]: - print("WARNING: {} will be ignored".format(key)) - else: - raise RuntimeError("failed to find config line for {}".format(key)) - - -def configure_top_level_key(lines, top_level_key, value): - for i, line in enumerate(lines): - if line.startswith("#" + top_level_key + " = ") or line.startswith( - top_level_key + " = " - ): - lines[i] = "{} = {}".format(top_level_key, to_toml(value)) - return - - raise RuntimeError("failed to find config line for {}".format(top_level_key)) - - -# Modify `sections` to reflect the parsed arguments and example configs. -def configure_file(sections, top_level_keys, targets, config): - for section_key, section_config in config.items(): - if section_key not in sections and section_key not in top_level_keys: - raise RuntimeError( - "config key {} not in sections or top_level_keys".format(section_key) - ) - if section_key in top_level_keys: - configure_top_level_key(sections[None], section_key, section_config) - - elif section_key == "target": - for target in section_config: - configure_section(targets[target], section_config[target]) - else: - configure_section(sections[section_key], section_config) - - -def write_uncommented(target, f): - block = [] - is_comment = True - - for line in target: - block.append(line) - if len(line) == 0: - if not is_comment: - for ln in block: - f.write(ln + "\n") - block = [] - is_comment = True - continue - is_comment = is_comment and line.startswith("#") - return f - - -def write_config_toml(writer, section_order, targets, sections): - for section in section_order: - if section == "target": - for target in targets: - writer = write_uncommented(targets[target], writer) - else: - writer = write_uncommented(sections[section], writer) - - def quit_if_file_exists(file): if os.path.isfile(file): msg = "Existing '{}' detected. Exiting".format(file) @@ -759,14 +734,14 @@ def quit_if_file_exists(file): # Parse all known arguments into a configuration structure that reflects the # TOML we're going to write out p("") - section_order, sections, targets = parse_args(sys.argv[1:]) + config = parse_args(sys.argv[1:]) # Now that we've built up our `config.toml`, write it all out in the same # order that we read it in. p("") p("writing `config.toml` in current directory") with bootstrap.output("config.toml") as f: - write_config_toml(f, section_order, targets, sections) + write_config_toml(f, config) with bootstrap.output("Makefile") as f: contents = os.path.join(rust_dir, "src", "bootstrap", "mk", "Makefile.in") From 986d8815b6225646d1a310327a4a34d407653bb7 Mon Sep 17 00:00:00 2001 From: abylaw Date: Tue, 25 Feb 2025 14:26:38 -0800 Subject: [PATCH 2/2] Fix formatting issues --- src/bootstrap/bootstrap_test.py | 22 ++++++++++++++++------ src/bootstrap/configure.py | 10 +++++++--- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/bootstrap/bootstrap_test.py b/src/bootstrap/bootstrap_test.py index b608dfb5168a7..521b5a8a8cc46 100644 --- a/src/bootstrap/bootstrap_test.py +++ b/src/bootstrap/bootstrap_test.py @@ -193,14 +193,24 @@ def test_set_target(self): ) def test_set_target_option(self): - build = serialize_and_parse(["--build=x86_64-unknown-linux-gnu", "--llvm-config=/usr/bin/llvm-config"]) + build = serialize_and_parse( + ["--build=x86_64-unknown-linux-gnu", "--llvm-config=/usr/bin/llvm-config"] + ) self.assertEqual( - build.get_toml("llvm-config", section="target.x86_64-unknown-linux-gnu"), "/usr/bin/llvm-config" + build.get_toml("llvm-config", section="target.x86_64-unknown-linux-gnu"), + "/usr/bin/llvm-config", ) def test_set_targets(self): # Multiple targets can be set - build = serialize_and_parse(["--set", "target.x86_64-unknown-linux-gnu.cc=gcc", "--set", "target.aarch64-apple-darwin.cc=clang"]) + build = serialize_and_parse( + [ + "--set", + "target.x86_64-unknown-linux-gnu.cc=gcc", + "--set", + "target.aarch64-apple-darwin.cc=clang", + ] + ) self.assertEqual( build.get_toml("cc", section="target.x86_64-unknown-linux-gnu"), "gcc" ) @@ -208,10 +218,10 @@ def test_set_targets(self): build.get_toml("cc", section="target.aarch64-apple-darwin"), "clang" ) - build = serialize_and_parse(["--target", "x86_64-unknown-linux-gnu,aarch64-apple-darwin"]) - self.assertNotEqual( - build.config_toml.find("target.aarch64-apple-darwin"), -1 + build = serialize_and_parse( + ["--target", "x86_64-unknown-linux-gnu,aarch64-apple-darwin"] ) + self.assertNotEqual(build.config_toml.find("target.aarch64-apple-darwin"), -1) self.assertNotEqual( build.config_toml.find("target.x86_64-unknown-linux-gnu"), -1 ) diff --git a/src/bootstrap/configure.py b/src/bootstrap/configure.py index 4df9f9b09a2df..387910ecfd2dc 100755 --- a/src/bootstrap/configure.py +++ b/src/bootstrap/configure.py @@ -374,6 +374,7 @@ def is_value_list(key): VERBOSE = False + # Parse command line arguments into a valid build configuration. def parse_args(args): known_args = validate_args(args) @@ -383,6 +384,7 @@ def parse_args(args): _set("build.configure-args", args, config) return config + # Validate command line arguments, throwing an error if there are any unknown # arguments, missing values or duplicate arguments when option-checking is also # passed as an argument. Returns a dictionary of known arguments. @@ -434,7 +436,7 @@ def validate_args(args): known_args[option.name] = [] elif option.name in known_args and option.name != "set": duplicate_args.append(option.name) - + known_args[option.name].append((option, value)) break @@ -595,7 +597,7 @@ def get_configured_targets(config): def write_block(f, config, block): last_line = block[-1] - key = last_line.split("=")[0].strip(' #') + key = last_line.split("=")[0].strip(" #") value = config[key] if key in config else None if value is not None: for ln in block[:-1]: @@ -624,10 +626,12 @@ def write_config_toml(f, config): section = [] block = [] + i = 0 # Drop the initial comment block - for i, line in enumerate(lines): + for line in lines: if not line.startswith("#"): break + i += 1 for line in lines[i:]: if line.startswith("["):