Skip to content

Rearrange config.codegen.toml to match config.compiler.toml #119040

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

Closed
wants to merge 2 commits into from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Dec 17, 2023

This makes it easier to compare the two profiles and see which parts are different.

No functional changes.

This makes it easier to see which parts of the two profiles are different.
This comment became obsolete after rust-lang#116881 switched over to
`download-ci-llvm = "if-unchanged"`.
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

r? @onur-ozkan

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs and change-id in config.example.toml.

This PR changes src/bootstrap/defaults/config.codegen.toml. If appropriate, please also update config.compiler.toml so the defaults are in sync.

@onur-ozkan
Copy link
Member

This makes it easier to compare the two profiles and see which parts are different.

I believe this should be handled manually by the developer (largest file is 24 lines anyway) since we can't keep all the default files synced.

These changes will hide the actual commits; checking the latest commit and its description of a line is often the preferred way to understand the reason for the existence of the line. Merging these changes doesn't seem worth in my opinion.

@Zalathar
Copy link
Contributor Author

For the specific lines being moved, the information currently exposed by blame seems much less valuable to me than helping humans spot the intentional differences and keep the two files in sync. (And the old blame information is one click away on GitHub.)

Anecdotally, I did find it difficult to manually identify the differences between the two files, especially when I was new to the codebase, which is what inspired me to make this change.

(Ultimately I don't plan to fight hard for this, but I do think it's a net improvement.)

@onur-ozkan
Copy link
Member

onur-ozkan commented Dec 17, 2023

For the specific lines being moved, the information currently exposed by blame seems much less valuable to me than helping humans spot the intentional differences and keep the two files in sync.

The problem you are trying to solve still exists on the other configurations. We can't keep them synced manually since there's no check in CI. The actual solution to this problem would be to sync all default files with each other (no just two of them). And implementing tests and blessings on top of this which will ensure the correctness of the synchronization. However, considering that the largest default file is only 24 lines, this doesn't seem worth the effort it requires. At the end this PR doesn't solve anything other than providing a temporary workaround for your usecase.

And the old blame information is one click away on GitHub

I typically avoid making changes to a line unless it's truly necessary and adds value. Because trying to trace the specific reason for a line in the commit history is much harder than getting the latest commit on the line.

In most cases, you don't need to open Github if you have git plugins installed on your editor.

e.g., from vscode:

image

e.g., from vim:

image

@onur-ozkan
Copy link
Member

Maybe it's just me being wrong. I am happy to leave this decision to another bootstrap member.

r? @rust-lang/bootstrap

@Mark-Simulacrum
Copy link
Member

I don't have a strong opinion either way, and given the opposition from others on the team I'm inclined to forgo this at this time.

That said, I've thought for a while that we may want to consolidate some of [llvm], [build], etc. blocks in configuration into a top-level block; especially if we do that it would be relatively straightforward to add # tidy-alphabetical-start or so markers to all of these files to continuously enforce consistency between them (modulo comments, of course).

That's more work though :)

Going to send it back to you, but if you feel strongly about this I think dropping by #t-infra/bootstrap on Zulip to discuss it is the best bet. Otherwise we can close.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2023
@Zalathar
Copy link
Contributor Author

Zalathar commented Jan 1, 2024

I think I'll close this for now.

I had thought of this as a small and (hopefully) uncontroversial improvement, but since it's not very important and there's reluctance from the team, I'm fine with just dropping it.

@Zalathar Zalathar closed this Jan 1, 2024
@Zalathar Zalathar deleted the codegen-profile branch January 1, 2024 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

4 participants