-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: add logging verbosity flags to cli #4471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I left one small comment, then good to merge
(need to run cargo fmt
too)
Looks good! We can def merge pending tests + formatting... |
Would be great to merge this! Let me know if you want a hand with the formatting? |
prqlc/prqlc/Cargo.toml
Outdated
@@ -62,6 +63,7 @@ strum_macros = "0.26.2" | |||
anyhow = {version = "1.0.82", features = ["backtrace"], optional = true} | |||
clap = {version = "4.4.18", features = ["derive", "env", "wrap_help"], optional = true} | |||
clap_complete_command = {version = "0.5.1", optional = true} | |||
clap-verbosity-flag = { version = "2.2.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of this seems to bring the MSRV to 1.74.
My question is that changing the MSRV of prqlc here will even affect its functionality as a built-in that does not require a
cli feature.
Why are cli features included in default? I think it would be better to exclude them from default.
(As a side note, I think the MSRV could be 1.74 for now. I just don't think it should be pulled to other parts of the cli feature about the future.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLI is in default otherwise cargo install prqlc
doesn't work, and that is likely to be done by many more people than adding prqlc as a library dependency.
We have changed this around a couple of times — having a separate crate etc — I do think this is the right tradeoff at the moment, even though it's indeed imperfect.
It does present an issue with defining the MSRV though, since we need to have a value in the crate and it can't be dependent on features. @eitsupi IIUC the R ecosystem & debian were the main impediments to increasing the MSRV; is that still the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-span if we downgrade to 2.1.2 then we get the 1.70.0 MSRV; let's do that I think. Then we'll upgrade when MSRVs match in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC the R ecosystem & debian were the main impediments to increasing the MSRV; is that still the case?
Over the past month, rustc on Debian has migrated rapidly from 1.70 to 1.74, and Ubuntu is currently at 1.75, so I consider it quite unlikely that setting 1.74 or 1.75 will cause problems.
The problem with CRAN is that it continues to use Fedora 36 (time stopped at rustc 1.69), which reached EOL a year ago, so it is not worth considering.
They do not have a roadmap for how long they will continue to use Fedora 36 and cannot predict the future.
Since they are volunteers and do not seem to have the resources to update Fedora under the current system, I suspect that this situation may continue until the infrastructure is completely rebuilt on an IaC basis.
Either way I will either patch to lower the MSRV (which may involve manual modification of the lock file and is a tremendous pain) or simply give up submitting to CRAN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK, that's a shame re CRAN, hope they figure out a way to go forwards.
Given they're at 1.69, I guess staying on 1.70 doesn't help much? And we can bump to 1.74? I think it's quite easy for us to stay at 1.70 (in this case a tiny downgrade), but sounds like that's not actually that helpful for CRAN?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think the process of lowering from 1.70 to 1.69 is much easier than the process of lowering from 1.74 to 1.69.
As far as I am aware there is no new syntax introduced in 1.70 in prqlc at the moment, just some dependencies.
If the checks in 1.70 were to be stopped, it would include many more dependencies, which would make it more difficult to identify the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's leave it at 1.70 for the moment then.
Let's do the downgrade and merge this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we aren't even on MSRV 1.70 right now:
~/projects/prql git:[main]
cargo msrv --linear
Fetching index
Determining the Minimum Supported Rust Version (MSRV) for toolchain aarch64-apple-darwin
Using check command cargo check
Check for toolchain '1.78.0-aarch64-apple-darwin' succeeded
Check for toolchain '1.77.2-aarch64-apple-darwin' succeeded
Check for toolchain '1.76.0-aarch64-apple-darwin' succeeded
Check for toolchain '1.75.0-aarch64-apple-darwin' succeeded
Check for toolchain '1.74.1-aarch64-apple-darwin' succeeded
Check for toolchain '1.73.0-aarch64-apple-darwin' failed with:
┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: package `pulldown-cmark v0.10.3` cannot be built because it requires rustc 1.74 or newer, while the currently active rustc version is 1.73.0 │
│ Either upgrade to rustc 1.74 or newer, or use │
│ cargo update -p pulldown-cmark@0.10.3 --precise ver │
│ where `ver` is the latest version of `pulldown-cmark` supporting rustc 1.73.0 │
└───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Finished The MSRV is: 1.74.1 ██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████ 00:00:03
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but yes, I did downgrade that package to 2.1.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we aren't even on MSRV 1.70 right now:
Please check CI, only prqlc is confirming MSRV.
prql/.github/workflows/tests.yaml
Lines 495 to 498 in 045c03d
- name: Verify minimum rust version — prqlc | |
# Ideally we'd check all crates, ref https://github.com/foresterre/cargo-msrv/issues/295 | |
working-directory: prqlc/prqlc | |
run: cargo msrv verify |
but yes, I did downgrade that package to
2.1.2
The lock file does not seem updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test still fails. Perhaps a patch version update of one of the dependencies has changed the MSRV.
I think you need to undo all the changes to the lock file and update it again.
Cargo has a slightly odd feature where it presumes Edit: @eitsupi looks like you need to give the "OK" on the changes1 Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Makes things a little easier to debug and trace