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

simple custom log macro to avoid additional dependencies #6315

Closed

Conversation

cre4ture
Copy link
Contributor

@cre4ture cre4ture commented May 1, 2024

draft 3 for #6284, alternative for #6313 and #6314

The main idea here is that we have a own log macro to avoid additional dependencies.

Same as with #6314, the idea is that the logs are not part of the stable code-base.
They would always be removed when debuggin is done.
This way, there is no explicit activation mechanism needed and the test-framework
can always specify the logfile-path via environmental variable.
It will check after execution if the logfile was written to and then dumps the content.

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

  • I still don't see a usecase, and
  • this would cause completely unnecessary computation/allocation when logging is switched off, and
  • it doesn't address the main usecase that I'm guessing ("want to debug in an eprintln-style during tests that check for stderr").

src/uucore/src/lib/lib.rs Outdated Show resolved Hide resolved
tests/common/util.rs Show resolved Hide resolved
@cre4ture cre4ture mentioned this pull request May 1, 2024
@cre4ture cre4ture force-pushed the feature/automatic_log_file_dump_3 branch from 42afcc4 to b738a27 Compare May 4, 2024 11:03
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

I still don't quite see the advantage of this. So you're suggesting that we change:

  • old strategy: just insert eprintln!() temporarily whenever you debug something, and delete it before pushing (and if you forget, CI will remind you)
  • new strategy: just insert uu_log!() temporarily whenever you debug something, and delete it before pushing (and if you forget, a weird environment variable will cause file writes in production that GNU doesn't do)

So what is the advantage then?

EDIT: Ah, I was hoping this would reset the "changes requested" status. I'm neutral on this PR, and don't understand how to tell github about it.

Copy link

github-actions bot commented May 4, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
GNU test failed: tests/du/bind-mount-dir-cycle. tests/du/bind-mount-dir-cycle is passing on 'main'. Maybe you have to rebase?

@cre4ture cre4ture force-pushed the feature/automatic_log_file_dump_3 branch from ad704ba to 4be36ac Compare May 4, 2024 13:07
@cre4ture
Copy link
Contributor Author

cre4ture commented May 4, 2024

I still don't quite see the advantage of this. So you're suggesting that we change:

  • old strategy: just insert eprintln!() temporarily whenever you debug something, and delete it before pushing (and if you forget, CI will remind you)

  • new strategy: just insert uu_log!() temporarily whenever you debug something, and delete it before pushing (and if you forget, a weird environment variable will cause file writes in production that GNU doesn't do)

So what is the advantage then?

EDIT: Ah, I was hoping this would reset the "changes requested" status. I'm neutral on this PR, and don't understand how to tell github about it.

Thanks for neutral rating.
I added now a feature toggle for the uu_log!().
Let me help you to see the benefit:

I'd formulate it like this:

  • old strategy: Insert eprintln!(), disable the stderr assert of the test and add an additional statement into the test that enusres the stderr is logged before the failing assert. All this needs to be reverted before pushing, and if you forget, CI is not telling you either.
  • new strategy: Insert uu_log!() and activate the "uu_log" feature in "src/uucore/Cargo.toml". No test-code needs to be modied. Testframework will automatically ensure that the content of the logfile is printed when the test is executed. Before you push, you remove the "uu_log" feature from "src/uucore/Cargo.toml" and you can optionally revert the uu_log!() calls. If you forget, human reviewer would easily spot the change in "src/uucore/Cargo.toml" in more than 90% of the cases. And if this is not yet enough, one could introduce a seperate test in the CI that is red if this feature is accidentially left over.

@sylvestre Can you please review this proposal. I think complexity-wise you probably would rather agree here.

@cre4ture cre4ture force-pushed the feature/automatic_log_file_dump_3 branch from 4be36ac to 4124d9d Compare May 4, 2024 13:34
Copy link

github-actions bot commented May 4, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

NOT AN APPROVAL, but I don't find a way to reset the "changes requested" status, so I do it like this.

@sylvestre
Copy link
Sponsor Contributor

I am sorry but I still don't see the benefit of logging in general ?!

Copy link

github-actions bot commented May 4, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cre4ture
Copy link
Contributor Author

cre4ture commented May 4, 2024

There are no remaining negative side effects left that I know of.
I explained the benefits.
These are convencience reasons only and are not mandatory, of course.
But I'm spending the efforts on this topic for a good reason.
I think it would significantly improve the debugging experience for the topics I'm looking at.
I'm sure that some more people like me will find it usefull.

@sylvestre
Copy link
Sponsor Contributor

Sure but you are repeating the what when I care about the why?

Copy link

github-actions bot commented May 4, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cre4ture
Copy link
Contributor Author

cre4ture commented May 4, 2024

Sure but you are repeating the what when I care about the why?

I apprechiate your quick response.
The issue that I have is, that some people apparently argue about the disadvantages of the "what" before they actually undestood or agreed on the "why". From my perspective the "why" is already explained in the issue #6284 and I repeated it in the message above (#6315 (comment)). I know that these are no super strong reasons, but I was expecting that the "why" is clear in general. At least I do not know what I could add.

@cre4ture cre4ture closed this Sep 14, 2024
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