Skip to content

configure.py: Write last key in each section #143606

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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lambdageek
Copy link
Contributor

@lambdageek lambdageek commented Jul 7, 2025

The loop that writes the keys in each section of bootstrap.toml accumulates all the commented lines before a given key and emits them when it reaches the next key in the section. This ends up dropping lines accumulated for the last key

Fixes #143605

The loop that writes the keys in each section of bootstrap.toml
accumulates all the commented lines before a given key and emits them
when it reaches the next key in the section.  This ends up dropping
lines accumulated for the last key
@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 7, 2025
@Kobzol
Copy link
Member

Kobzol commented Jul 7, 2025

r? kobzol

@lolbinarycat does it look right? :)

@rustbot rustbot assigned Kobzol and unassigned jieyouxu Jul 7, 2025
@lolbinarycat
Copy link
Contributor

I only changed parse_example_config so I don't think this is right.

@lolbinarycat
Copy link
Contributor

lolbinarycat commented Jul 7, 2025

Oh! I believe this is kinda an existing bug that just so happened to work before because the last block in a section was always the comments for the next section, while now those comments are considered technically part of the first item in a block.

Still, I'm not sure if any other user of write_uncommented is relying on this behavior, and evidently we don't have enough tests to catch a lot of edge cases...

It would arguably be safer to modify parse_example_config instead to add back the empty sections, then document this quirk on write_uncommented, but that's not exactly a satisfying solution...

In any case it would be nice to give write_uncommented a docstring if possible.

@Kobzol I'll leave it up to you to make the final call, should we do this the "right" way (what the PR does now), or the way that is less likely to cause further regressions (making parse_example_config produce dummy blocks)?

@rust-log-analyzer

This comment has been minimized.

@lambdageek lambdageek force-pushed the configure-write-last-key branch from 4ed5f06 to b6d2130 Compare July 7, 2025 19:56
@Kobzol
Copy link
Member

Kobzol commented Jul 8, 2025

Thanks for taking a look. I examined the function in detail and determined that it is too magical to be understood 😆 I tried to rewrite it like this, which hopefully makes it clearer:

def write_uncommented(target, f):
     """Writes each block in 'target' that is not composed entirely of comments to 'f'.
    A block is a sequence of non-empty lines separated by empty lines.
    """
    block = []

    def flush(last):
        # If the block is entiry made of comments, ignore it
        entire_block_comments = all(ln.startswith("#") or ln == "" for ln in block)
        if not entire_block_comments and len(block) > 0:
            for line in block:
                f.write(line + "\n")
            # Required to output a newline before the start of a new section
            if last:
                f.write("\n")
        block.clear()

    for line in target:
        block.append(line)
        if len(line) == 0:
            flush(last=False)

    flush(last=True)
    return f

What do you think of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configure.py doesn't write out the last set key in a section
6 participants