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

doc: make environment variable setting more explicit. #5822

Merged
merged 1 commit into from
Feb 17, 2024
Merged

doc: make environment variable setting more explicit. #5822

merged 1 commit into from
Feb 17, 2024

Conversation

rzhb
Copy link
Contributor

@rzhb rzhb commented Jul 11, 2023

$env:RUSTFMT_LOG=rustfmt=DEBUG didn't work in PowerShell, $env:RUSTFMT_LOG="rustfmt=DEBUG" worked.

@ytmimi
Copy link
Contributor

ytmimi commented Jul 11, 2023

@rzhb Thanks for reaching out. Can you confirm what version of rustfmt you're using? If you're using rustfmt 1.6.0 we had an issue when migrating to tracing for logging (#5800), but that was resolved in source already (#5818)

@calebcartwright do you think we can do a subtree-sync soon to resolve this?

@rzhb
Copy link
Contributor Author

rzhb commented Jul 11, 2023

@ytmimi Maybe we have some misunderstand. This is not about rustfmt, just about environment variable setting in PowerShell and Windows.
In Linux, export RUSTFMT_LOG=rustfmt=DEBUG is same with export RUSTFMT_LOG="rustfmt=DEBUG".

But in Windows and PowerShell, $env:RUSTFMT_LOG=rustfmt=DEBUG gives error rustfmt=DEBUG: The term 'rustfmt=DEBUG' is not recognized as a name of a cmdlet, function, script file, or executable program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
I am using rustfmt 1.6.0-nightly (cb80ff13 2023-07-07)

@ytmimi
Copy link
Contributor

ytmimi commented Jul 11, 2023

@rzhb Yeah, I think there was some misunderstanding. I think I just had the logging issue top of mind so I assumed you had run into an issue. As mentioned above, logging is broken in rustfmt 1.6.0 so I wouldn't expect you to see any log output even when setting the env variable correctly. You might want to test logging on a previous version of rustfmt.

Contributing.md Outdated
@@ -59,7 +59,7 @@ example, the `issue-1111.rs` test file is configured by the file
## Debugging

Some `rewrite_*` methods use the `debug!` macro for printing useful information.
These messages can be printed by using the environment variable `RUSTFMT_LOG=rustfmt=DEBUG`.
These messages can be printed by using the environment variable `RUSTFMT_LOG="rustfmt=DEBUG"`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now I'm wondering if it would be better to mention RUSTFMT_LOG=rustfmt=DEBUG on linux and add a new sentence to make a distinction for RUSTFMT_LOG="rustfmt=DEBUG" when using powershell on windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value is a String, it needs the quote. "RUSTFMT_LOG=rustfmt=DEBUG" is just lazy and not formal.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rzhb first I want to highlight that I appreciate you helping to improve the docs, but I don't find the criticism to be constructive.

I wouldn't characterize this as lazy since the quotes are unnecessary when working on linux / mac. Given that the current docs are valid for some systems, I suggest making an amendment to the docs that informs powershell users that they need to wrap rustfmt=DEBUG in quotes. Are the quotes also necessary when running in the windows command prompt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Linux, without quotes works only because there has no whitespace or special characters.
It's better to always quote string for safety.

powershell
$env:RUSTFMT_LOG="rustfmt=DEBUG" work.
$RUSTFMT_LOG="rustfmt=DEBUG" didn't.

cmd
set RUSTFMT_LOG="rustfmt=DEBUG" didn't work. rustfmt has warning warning: invalid logging spec 'DEBUG"', ignoring it
set RUSTFMT_LOG=rustfmt=DEBUG work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for looking into this. From my understanding of your last comment the quotes are only required in powershell, and it seems like the quotes actually break things on cmd. Assuming that's the case I now feel more strongly about leaving the existing language as is and adding a note for powershell users.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I am fully in agreement with Yacin though, and don't want to move forward with the changes as proposed.

That being said, I'd also agree there's opportunities to improve things. First and foremost, the debug logging should be able to be enabled via the more simple RUSTFMT_LOG=debug.

The advanced filtering options are powerful, and certainly useful in more complex scenarios like the rustc codebase where you basically never want to build/test/troubleshoot with debugging enabled across the entire compiler, but instead want to only enable it for a subset, like a module.

I'd be completely open to simplifying the text to reflect this, which fwiw could obviate the topic of quotes. Then if necessary, we can mention the filtering feature, with or without examples

@ytmimi ytmimi dismissed calebcartwright’s stale review February 17, 2024 17:07

I went ahead and changed the docs to read RUSTFMT_LOG=debug as Caleb suggested

@ytmimi ytmimi merged commit b5dcc6f into rust-lang:master Feb 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants