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.

Sign up for GitHub

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

Jump to bottom

Support specify timings in .cargo/config #10567

Closed
wants to merge 1 commit into from
Closed

Support specify timings in .cargo/config #10567

wants to merge 1 commit into from

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Apr 14, 2022 •
edited
Loading

What does this PR try to resolve?

close #10509

Support specify timings in .cargo/config.

How should we test and review this PR?

  • unit test

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2022
@Rustin170506

This comment was marked as outdated.

Sign in to view
@Rustin170506

This comment was marked as outdated.

Sign in to view
@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2022
@Rustin170506 Rustin170506 force-pushed the rustin-patch-timing-config branch 6 times, most recently from ef55997 to e67f214 Compare April 15, 2022 14:54
@Rustin170506
Copy link
Member Author

@ehuss This PR is still being worked on, but before I add the documentation, I wanted to confirm with you that because these two options are not stable, I've marked the configuration as unstable as well. Is this correct? I want to make sure I'm on the right track. Could you please take a look? Thanks!

@Rustin170506 Rustin170506 force-pushed the rustin-patch-timing-config branch 2 times, most recently from 29a131d to a48b1c7 Compare April 15, 2022 15:08
@ehuss
Copy link
Contributor

ehuss commented Apr 18, 2022

Oh, sorry for the confusion. I don't think there is any need to make the config value itself unstable. It is pretty simple and straightforward.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-timing-config branch 2 times, most recently from 1fb5cb4 to 4e69e95 Compare April 19, 2022 12:47
@Rustin170506 Rustin170506 changed the title WIP: support specify timings in .cargo/config Support specify timings in .cargo/config Apr 19, 2022
@Rustin170506 Rustin170506 marked this pull request as ready for review April 19, 2022 12:59
@Rustin170506

This comment was marked as outdated.

Sign in to view
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Apr 19, 2022
@Rustin170506
Copy link
Member Author

Oh, sorry for the confusion. I don't think there is any need to make the config value itself unstable. It is pretty simple and straightforward.

@ehuss Thank you for your reply! I have deleted it. This PR is ready to be reviewed.

ehuss
ehuss reviewed Apr 21, 2022
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Just need some updates to make sure the JSON option isn't stabilized.

src/doc/src/reference/timings.md Outdated Show resolved Hide resolved
src/doc/src/reference/config.md Outdated Show resolved Hide resolved
src/cargo/util/command_prelude.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 requested a review from ehuss April 22, 2022 12:36
@Rustin170506 Rustin170506 force-pushed the rustin-patch-timing-config branch 3 times, most recently from 8665669 to d6b6f8d Compare April 23, 2022 15:09
@ehuss ehuss added the T-cargo Team: Cargo label Apr 25, 2022
@ehuss
Copy link
Contributor

ehuss commented Apr 25, 2022

Thanks! Can you squash the commits?

@rust-lang/cargo This is a proposal to add and stabilize the build.timings config setting which provides a way to enable the timings report via config. This can be easier to use in a complex build system, or just to avoid passing the CLI option frequently.

[build]
timings = ["html"]

or CARGO_BUILD_TIMINGS=html environment variable.

The config uses an array. "html" is the only stable option. "json" is unstable. There could be other considerations for the type of the value (like a boolean or a string), but I feel like an array is the simplest, and is flexible. If more complex options are needed in the future, then the type can be overloaded.

I don't feel there is too much to consider, so I don't feel like it needs to be unstable.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 25, 2022 •
edited by weihanglo
Loading

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Apr 25, 2022
@Rustin170506
Copy link
Member Author

Thanks! Can you squash the commits?

Done.

@joshtriplett
Copy link
Member

--timings seems like a debugging option; what's the use case for enabling it in a config file?

@Rustin170506
Copy link
Member Author

--timings seems like a debugging option; what's the use case for enabling it in a config file?

Debug and optimize many times in the development process?

@Rustin170506
Copy link
Member Author

Also, I think this configuration would be helpful if there is a continuous integration system that is constantly getting timings reports.

@Rustin170506
Copy link
Member Author

@Eh2406 @epage Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

@bors
Copy link
Contributor

bors commented Jun 15, 2022

☔ The latest upstream changes (presumably #10753) made this pull request unmergeable. Please resolve the merge conflicts.

@Rustin170506
Support specify timings in .cargo/config
34a2496 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@Rustin170506
Copy link
Member Author

The conflict has been resolved and if anyone can help move it forward, I would appreciate it.

@epage
Copy link
Contributor

epage commented Jul 5, 2022

This feels like something more transient. The motivations listed in this PR sound more like speculation, so I reached out on the original Issue to the filer for what their intended motivation was.

@Rustin170506
Copy link
Member Author

so I reached out on the original Issue to the filer for what their intended motivation was.

He said he just wanna set up all things in the config. What do you think? Should I close this PR?

This can be easier to use in a complex build system, or just to avoid passing the CLI option frequently.

Also, what do you think of this idea? Do you think it's useful?

@Rustin170506
Copy link
Member Author

I am going to close this PR as the motivation is not clear. Thanks for your review and feedback.

@rfcbot rfcbot removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss Awaiting requested review from ehuss

@weihanglo weihanglo Awaiting requested review from weihanglo

Assignees

@ehuss ehuss

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Team: Cargo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot specify timings in .cargo/config
9 participants
@Rustin170506 @rust-highfive @ehuss @rfcbot @joshtriplett @bors @epage @weihanglo @rustbot