Skip to content
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

[107049][Easy] Modify readme to suggest using configure #108641

Closed
wants to merge 3 commits into from

Conversation

lionellloh
Copy link
Contributor

@lionellloh lionellloh commented Mar 2, 2023

This is related to #107050. Modifying instructions in readme to use configure instead of printf

Manual Test:

Command

lionellloh@lionellloh-mbp rust % ./configure --set changelog-seen=1 --set profile=user                   
configure: processing command line
configure: 
configure: changelog-seen       := 1
configure: profile              := user
configure: build.configure-args := ['--set', 'changelog-seen=1', '--set', 'profil ...
configure: 
configure: writing `config.toml` in current directory
configure: 
configure: run `python /Users/lionellloh/rust/x.py --help`

Result

16c16
< changelog-seen = 1
---
> changelog-seen = 2
26c26
< profile = user
---
> #profile = <none>
331c331
< configure-args = ['--set', 'changelog-seen=1', '--set', 'profile=user']
---
> #configure-args = []
678c678
< [target.x86_64-apple-darwin]
---
> [target.x86_64-unknown-linux-gnu]
812d811
< 

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023
@lionellloh lionellloh changed the title [107050][Easy] Modify readme to suggest using configure [107049][Easy] Modify readme to suggest using configure Mar 2, 2023
@albertlarsan68
Copy link
Member

I think this should be changed to suggest using ./x setup user

@lionellloh
Copy link
Contributor Author

I think this should be changed to suggest using ./x setup user

Ooh do you mind explaining why so?

I am new so just passing the message from #107049 from @jyn514

We should make it possible to set top-level keys through configure, both changelog-seen and profile. We should also update the line in the readme linked above to suggest ./configure instead of printf.

@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@albertlarsan68 they are not quite the same. x setup user requires building bootstrap first and sometimes distros want to apply configuration when building bootstrap itself.

@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 2, 2023

📌 Commit dccd4ec has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023
README.md Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2023
@albertlarsan68
Copy link
Member

Then I think there should be a section for the distros, and one for the users wanting to build from source.

Remove accidentally inserted line

Co-authored-by: jyn <[email protected]>
@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

I don't really want to add more blessed paths 😓 bootstrap has enough configuration knobs as-is.

What are you hoping to gain from having people run x setup instead?

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Wait, so I read the original issue and this doesn't actually do what I want - I want ./configure to set profile=user by default, even if --set profile isn't passed.

README.md Outdated Show resolved Hide resolved
@albertlarsan68
Copy link
Member

In one of the guides, it is recommended that x is used rather than configure, since the makefile it produces is just a wrapper around x.

@lionellloh
Copy link
Contributor Author

I think I have applied the change requested @jyn514

@jyn514
Copy link
Member

jyn514 commented Mar 4, 2023

Wait, so I read the original issue and this doesn't actually do what I want - I want ./configure to set profile=user by default, even if --set profile isn't passed.

@lionellloh I don't see this change.

@jyn514
Copy link
Member

jyn514 commented Mar 4, 2023

In one of the guides, it is recommended that x is used rather than configure, since the makefile it produces is just a wrapper around x.

Do you have a link to this? I vaguely remember some text saying "configure generates config.toml", but not a recommendation against using it.

@jyn514
Copy link
Member

jyn514 commented Apr 9, 2023

Hi @lionellloh, it's been a while - do you know when you'll have time to look at this again? Is it clear what to do?

@jyn514
Copy link
Member

jyn514 commented Apr 19, 2023

Going to close this in favor of #110541.

@jyn514 jyn514 closed this Apr 19, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 19, 2023
Fix various configure bugs

Fixes rust-lang#107050. Fixes rust-lang#108928. Closes rust-lang#108641.

I recommend reading this commit-by-commit to see the commit descriptions, but the code changes are small.

This also changes the README to suggest `configure` instead of `printf`, as well as a few other cleanups described in the commit message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants