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

./configure --set changelog-seen=2 doesn't work #107049

Closed
jyn514 opened this issue Jan 18, 2023 · 14 comments · Fixed by #108229
Closed

./configure --set changelog-seen=2 doesn't work #107049

jyn514 opened this issue Jan 18, 2023 · 14 comments · Fixed by #108229
Assignees
Labels
C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@jyn514
Copy link
Member

jyn514 commented Jan 18, 2023

; ./configure --set changelog-seen=2
configure: processing command line
configure: 
configure: changelog-seen       := 2
configure: build.configure-args := ['--set', 'changelog-seen=2']
Traceback (most recent call last):
  File "/Users/jyn/src/rust/./src/bootstrap/configure.py", line 465, in <module>
    raise RuntimeError("config key {} not in sections".format(section_key))
RuntimeError: config key changelog-seen not in sections

The problem is that the configure script assumes that all keys are in a section and there are no top-level keys. This forces people to use printf instead:

rust/README.md

Lines 89 to 96 in 37c1d6d

The Rust build system uses a file named `config.toml` in the root of the
source tree to determine various configuration settings for the build.
Set up the defaults intended for distros to get started. You can see a full list of options
in `config.toml.example`.
```sh
printf 'profile = "user" \nchangelog-seen = 2 \n' > config.toml
```

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 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. labels Jan 18, 2023
@jyn514 jyn514 added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. labels Jan 18, 2023
@jyn514
Copy link
Member Author

jyn514 commented Jan 18, 2023

Mentoring instructions: look around

for section_key in config:
section_config = config[section_key]
if section_key not in sections:
raise RuntimeError("config key {} not in sections".format(section_key))
if section_key == 'target':
for target in section_config:
configure_section(targets[target], section_config[target])
else:
configure_section(sections[section_key], section_config)

I think the fix would be something like if section_config is None: # special-casing for top-level keys.

@jyn514 jyn514 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jan 18, 2023
@shourya5
Copy link
Contributor

@claim

@shourya5
Copy link
Contributor

In the configure.py, from where exactly do we get the top-level keys? from the set function?

@jyn514
Copy link
Member Author

jyn514 commented Jan 19, 2023

In the configure.py, from where exactly do we get the top-level keys? from the set function?

I'm not sure off the top of my head. How could you find out that information?

@lionellloh
Copy link
Contributor

@shourya5 Hi are you still working on this?

@zephaniahong
Copy link
Contributor

@jyn514 do you think it's okay to unassign @shourya5 ?
This issue is blocking #107050 as well.

@zephaniahong
Copy link
Contributor

@lionellloh Since there's no response from the assignee, I think you can take the issue. Are you still keen on working on this issue?

@lionellloh
Copy link
Contributor

lionellloh commented Feb 17, 2023

Yup I am interested! @rustbot claim

@rustbot rustbot assigned lionellloh and unassigned shourya5 Feb 17, 2023
@lionellloh
Copy link
Contributor

lionellloh commented Feb 18, 2023

Hi @zephaniahong and @jyn514, thanks for mentoring!

I think the fix would be something like if section_config is None: # special-casing for top-level keys.

Assuming I run ./configure --set changelog-seen=2, I think section_config will be 2. So I am not sure if this path would work.

It seems like the first blocker is that sections does not contain changelog-seen or profile as keys. Should the approach be to expand sections with those keys? Or should we just allow changelog-seen / profile as special cases? I am guessing it's closer to the latter, since that might affect the definition of sections which searches for [.

Finally, I am a little confused what top-level keys mean - do they exclusively refer to [changelog-seen, profile] ? Are there more?

@lionellloh
Copy link
Contributor

I am able to expand sections: dict when parsing to include profile and changelog-seen but I realise I may either have to make changes to def configure_section() or write a new method.

Before I do that, I would like to understand how the configure script is commonly used. Could you give me some sample commands, say if I want to modify the skip-rebuild field of llvm?

@lionellloh
Copy link
Contributor

Shall we refactor the 2 top level keys under a [general section? this makes the parsing logic more consistent and unwieldy lol

@zephaniahong
Copy link
Contributor

Finally, I am a little confused what top-level keys mean - do they exclusively refer to [changelog-seen, profile] ? Are there more?

Top level keys are keys without any header like [rust] or [llvm] etc.
you can take a look at config.toml.example for a complete example of what a configuration could look like. I think that would resolve most, if not all of your queries(:

Hope this helps!

@zephaniahong
Copy link
Contributor

I am able to expand sections: dict when parsing to include profile and changelog-seen but I realise I may either have to make changes to def configure_section() or write a new method.

Before I do that, I would like to understand how the configure script is commonly used. Could you give me some sample commands, say if I want to modify the skip-rebuild field of llvm?

try running ./configure --help

@lionellloh
Copy link
Contributor

Got it, thanks @zephaniahong. I put up a PR. This is kinda my first PR so would appreciate all the tips and advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants