search

travitch / build-bom

Dynamically discover the commands used to create a piece of software
Apache License 2.0
45 stars 8 forks source link

Use stderr when reporting errors #56

Closed kquick closed 4 months ago

travitch commented 4 months ago

I'm really surprised I used println. I'm not sure what came over me. I don't want to put a bigger change on you, but what do you think of switching to a proper logger with env_logger (https://crates.io/crates/env_logger), which is the easy top-level driver bit for the log crate, which provides proper error!, warn! and info! macros?

travitch commented 4 months ago

That might also subsume #55 - we could use info/warn/error

kquick commented 4 months ago

That sounds reasonable, I can make that change. I had somewhat subconsciously been worried about facing the plethora of logger choices that we had with Haskell, but our needs are light and this seems like a reasonable library.

My intended changes would be:

  1. Parse cmdline to get verbosity settings
  2. init the env_logger based on the verbosity settings (env can override). Verbosity 0 would be warn, verbosity 1 would be info, and verbosity 2 would be debug. Env var filter name would be "BUILD_BOM_LOG" because this is a wrapper that might invoke other rust apps unless you'd prefer sticking with the default "RUST_LOG".
  3. env_logger always writes to stderr, so I would not use it for printing the summary if enabled (https://github.com/travitch/build-bom/pull/55/files#diff-08b7179a709f2ecef259a56c08e59ab27f46d9a8073a5e7dde28fffe9e532d3aR190).
travitch commented 4 months ago

I think we can just stick with RUST_LOG for now. It is a good point that it can invoke other rust programs. I know the cargo build currently freaks out under build-bom.

kquick commented 4 months ago

OK, this is ready to merge again. This includes #55 (because the changes overlapped) and 55 should be abandoned.

I had to create run_bom_command as an entrypoint for testing because the test framework already establishes a logger and so run_bom's re-attempt to install a logger would fail. If you would prefer, I can add code instead that handles the duplicate logger initialization failure, but that seemed like awkward code for a situation that should only happen in a testing environment.

kquick commented 4 months ago

[btw, for whatever reason I do not have github privs to select you--or anyone--as a reviewer.]