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

don't log to stdout if not cli #708

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Dauaaa
Copy link

@Dauaaa Dauaaa commented Dec 2, 2024

This PR is a proposal to solve #706. I understand there are a lot of ways to do this so I just tried a simple approach.

I did this by adding a static boolean that will be set to true in main.rs making the program output to stdout (current behavior). Otherwise (if boolean is not set) the old logs will use log::$level.

I used #[doc(hidden)] attribute to avoid flooding the API documentation with print related utilities.


Extra: removed once_cell dependency

@bee-san
Copy link
Owner

bee-san commented Dec 2, 2024

Running CI now, this PR looks cool 🔥

@Dauaaa Dauaaa force-pushed the discriminate-cli-and-lib-mode branch from e144fe2 to b3923d7 Compare December 2, 2024 16:32
@Dauaaa
Copy link
Author

Dauaaa commented Dec 2, 2024

@bee-san didn't catch the clippy warning because my clippy was not updated.

I fixed the error, please try running CI again :)

@bee-san
Copy link
Owner

bee-san commented Dec 3, 2024

I am also thinking about whether or not it's worth testing for this. We could write a test similar to https://github.com/RustScan/RustScan/blob/master/tests/timelimits.rs that runs rustscan as the api (2 files?) and if it logs to stdout, the test fails 🤔

@Dauaaa Dauaaa requested a review from PsypherPunk December 3, 2024 15:40
@Dauaaa
Copy link
Author

Dauaaa commented Dec 3, 2024

@bee-san I added a test to check if library outputs to stdout, I used an unstable feature for tests only.

The other options I thought of:

  • build another binary that doesn't call set_cli_mode and do something similar to the timelimits test
  • add third party dependencies that does something similar to what I did

@bee-san
Copy link
Owner

bee-san commented Dec 5, 2024

The ci is failing because Just is unhappy:

cargo nextest run
8
    Updating crates.io index
9
   Compiling rustscan v2.3.0 (/Users/runner/work/RustScan/RustScan)
10
error[E0554]: `#![feature]` may not be used on the stable release channel
11
 --> tests/log_mode.rs:4:1
12
  |
13
4 | #![feature(internal_output_capture)]
14
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is because the CI uses the stable release channel of Rust, may need to update the workflow :)

Thanks so much for the test!!

@Dauaaa Dauaaa force-pushed the discriminate-cli-and-lib-mode branch from db0992c to 42eff43 Compare December 6, 2024 22:49
@Dauaaa
Copy link
Author

Dauaaa commented Dec 6, 2024

@bee-san Because I can't use unstable rust, I created a rust example to test, I used example to avoid having binaries that could be confused for the actual program. Simple explanation about how examples work in rust.

I actually read the complete contributing.md and ran cargo test inside the docker container, it worked. So I'm pretty sure it won't error this time. I'm just not sure you're satisfied with the current approach for testing 😅.

Sorry for delay, I was a bit crunched this week so took me a while to respond.

@bee-san
Copy link
Owner

bee-san commented Dec 9, 2024

It's okay! It's open source, take as long as you need.

With that in mind, I'd need to read the docs on some of these functions. Give me a bit of time :)

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