feat: add logging verbosity flags to cli #4471

m-span · 2024-05-14T02:39:12Z

Makes things a little easier to debug and trace

max-sixty

Looks good!

I left one small comment, then good to merge

(need to run cargo fmt too)

prqlc/prqlc/src/cli/mod.rs

prqlc/prqlc/Cargo.toml

max-sixty · 2024-05-16T07:10:39Z

Looks good! We can def merge pending tests + formatting...

max-sixty · 2024-05-29T06:10:14Z

Would be great to merge this! Let me know if you want a hand with the formatting?

eitsupi · 2024-05-29T14:35:48Z

prqlc/prqlc/Cargo.toml

@@ -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 }


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.)

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?

@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.

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.

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?

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.

OK, let's leave it at 1.70 for the moment then.

Let's do the downgrade and merge this PR!

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

but yes, I did downgrade that package to 2.1.2

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.

prqlc/prqlc/src/cli/mod.rs

eitsupi

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.

max-sixty · 2024-06-01T06:08:45Z

Cargo has a slightly odd feature where it presumes ~ in requirements. So I pinned and pushed. Sorry we got bogged down a bit in the overhead on this one. The feature is nice!

Edit: @eitsupi looks like you need to give the "OK" on the changes¹

maybe we shouldn't use this feature unless something is really important and won't get checked in tests (which this will), since it blocks until the original individual approves... ↩

eitsupi

Thanks!

m-span mentioned this pull request May 14, 2024

refactor: more error info, span merge #4470

Open

max-sixty approved these changes May 14, 2024

View reviewed changes

prqlc/prqlc/src/cli/mod.rs Outdated Show resolved Hide resolved

max-sixty reviewed May 14, 2024

View reviewed changes

prqlc/prqlc/Cargo.toml Outdated Show resolved Hide resolved

max-sixty approved these changes May 16, 2024

View reviewed changes

m-span force-pushed the cli-verbose branch from 9728089 to bad23fc Compare May 29, 2024 14:26

eitsupi reviewed May 29, 2024

View reviewed changes

m-span force-pushed the cli-verbose branch from bad23fc to 2adc293 Compare May 29, 2024 14:43

max-sixty reviewed May 29, 2024

View reviewed changes

prqlc/prqlc/src/cli/mod.rs Show resolved Hide resolved

m-span added 6 commits May 30, 2024 19:45

feat: add logging verbosity flags to cli

d95954b

cargo fixup

1917059

remove trace cause backtrace

53bb850

formatting

6fd38d1

downgrade clap-verbosity-flag to maintain MSRV

62fab68

update lockfile

9048e1f

m-span force-pushed the cli-verbose branch from 7c93541 to 9048e1f Compare May 30, 2024 23:48

eitsupi requested changes May 31, 2024

View reviewed changes

Pin clap-verbosity-flag

8ef365a

max-sixty enabled auto-merge (squash) June 1, 2024 06:08

max-sixty approved these changes Jun 1, 2024

View reviewed changes

eitsupi approved these changes Jun 1, 2024

View reviewed changes

7215738

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add logging verbosity flags to cli #4471

feat: add logging verbosity flags to cli #4471

m-span commented May 14, 2024

max-sixty left a comment •

edited

max-sixty commented May 16, 2024

max-sixty commented May 29, 2024

eitsupi May 29, 2024

max-sixty May 29, 2024

max-sixty May 29, 2024

eitsupi May 29, 2024

max-sixty May 29, 2024

eitsupi May 29, 2024

max-sixty May 29, 2024

m-span May 30, 2024

m-span May 30, 2024

eitsupi May 30, 2024

eitsupi left a comment

max-sixty commented Jun 1, 2024 •

edited

eitsupi left a comment

	- 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

feat: add logging verbosity flags to cli #4471

Are you sure you want to change the base?

feat: add logging verbosity flags to cli #4471

Conversation

m-span commented May 14, 2024

max-sixty left a comment • edited

Choose a reason for hiding this comment

max-sixty commented May 16, 2024

max-sixty commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eitsupi left a comment

Choose a reason for hiding this comment

max-sixty commented Jun 1, 2024 • edited

Footnotes

eitsupi left a comment

Choose a reason for hiding this comment

max-sixty left a comment •

edited

max-sixty commented Jun 1, 2024 •

edited