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

Use Windows Terminal to display DT stdout/stderr #17141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsmucr
Copy link

@jsmucr jsmucr commented Jul 14, 2024

Currently on Windows, DT redirects all output to

%LOCALAPPDATA%\Microsoft\Windows\INetCache\darktable\darktable-log.txt

I suggest to override this behavior for Windows Terminal which is the default terminal app since Windows 11, assuming that the user runs DT in a terminal for debugging reasons.

It is however still perfectly possible to redirect DT output using the standard redirection operator:

darktable\bin\darktable.exe -d lua > darktable.log 2>&1

@jsmucr
Copy link
Author

jsmucr commented Jul 14, 2024

Please take this as a suggestion. I presume there's more to change, but I didn't dig deeper yet.

@TurboGit
Copy link
Member

TurboGit commented Aug 6, 2024

@wpferguson : Maybe you can comment on this Windows specific PR? TIA.

@TurboGit TurboGit added this to the 5.0 milestone Aug 6, 2024
@TurboGit TurboGit added the feature: redesign current features to rewrite label Aug 6, 2024
@wpferguson
Copy link
Member

I'll look at it...

@wpferguson
Copy link
Member

wpferguson commented Aug 7, 2024

Tested on Win 11. Running in terminal works fine (actually better because it eliminates the "flashing" windows problems). Running in command prompt still works normally.

I'll test on Win 10 and see what happens.

EDIT: Tested on Win 10. Has no effect, even after forcing the terminal application to be the command processor. Had no negative effects either.

I don't have a windows 8 or 8.1 machine so can't test but it shouldn't have any negative effects.

@victoryforce
Copy link
Collaborator

Tested on Win 11. Running in terminal works fine (actually better because it eliminates the "flashing" windows problems). Running in command prompt still works normally.

That is, running in the Windows Terminal redirects the output, but running directly in the command processor (without the participation of Windows Terminal) does not?

I'll test on Win 10 and see what happens.

EDIT: Tested on Win 10. Has no effect, even after forcing the terminal application to be the command processor. Had no negative effects either.

I didn't quite understand why that could be. If we start Windows Terminal and run darktable in it, it doesn't matter if it happens in Windows 11 or Windows 10. The code checks for the presence of the WT_SESSION environment variable, the presence of which does not depend on the version of Windows.

I don't have a windows 8 or 8.1 machine so can't test but it shouldn't have any negative effects.

Yes, it is guaranteed for the suggested code because Windows 8.x does not support Windows Terminal.

@wpferguson
Copy link
Member

That is, running in the Windows Terminal redirects the output, but running directly in the command processor (without the participation of Windows Terminal) does not?

Running in Windows Terminal lets the output come to the window. Running in a command prompt redirects the output to a file, as currently happens.

I didn't quite understand why that could be.

The terminal app in win 10 wasn't listed as Windows Terminal. Perhaps it needs to be installed separately? Just checked and that is the case, it doesn't come as part of windows 10.

@jsmucr
Copy link
Author

jsmucr commented Aug 9, 2024 via email

@victoryforce
Copy link
Collaborator

That is, running in the Windows Terminal redirects the output, but running directly in the command processor (without the participation of Windows Terminal) does not?

Running in Windows Terminal lets the output come to the window. Running in a command prompt redirects the output to a file, as currently happens.

OK, as it should be (I asked for clarification because the original wording could be interpreted in different ways).

Oops, just now noticed that my question was the opposite of the correct wording! :)

I didn't quite understand why that could be.

The terminal app in win 10 wasn't listed as Windows Terminal. Perhaps it needs to be installed separately? Just checked and that is the case, it doesn't come as part of windows 10.

So I didn't understand correctly, I thought that the comment meant that "Windows Terminal" was launched in Windows 10. I have it installed in Windows 10. But it's hard for me to remember, maybe I actually installed it manually once for testing after seeing it advertised somewhere...

But that's really irrelevant, since we definitely don't need to be tied to the Windows Terminal specifically, even if we go in the direction suggested in this PR. But here we need a more detailed description of the context and what problem we are solving. This will be quite a lot of text, so the continuation is in the following comments.

@victoryforce
Copy link
Collaborator

@jsmucr First of all, thank you for your desire to improve darktable and your contribution!

Indeed, it is reasonable to assume that if a user on Windows runs a program not through an icon, but from the command line in the terminal, then there are chances than this is done for the purpose of debugging. But this is not always the case. This may be an advanced user who needs it to specify darktable command line options. For example, to specify a different configuration directory.

As for the specific proposal implemented by this PR, it has two drawbacks:

  • We should not limit ourselves to Windows Terminal, any console is good enough to display debug output
  • We should not limit disabling redirection to the log file only if there are enabled debug channels

Now in more detail:

  • "Let's assume that Windows Terminal is good enough to display debug output" - any terminal is good enough to display debug output. So instead of checking if we are running from Windows Terminal we need to check if we are running from any console. I believe this can be done by checking for SESSIONNAME environment variable.

  • Output redirection is suggested to be turned off only when the arguments include activation of debug channels (-d/--debug). But some debug printing can occur even in the absence of these arguments (this is called DT_DEBUG_ALWAYS in the code). And in this case, the program prints a warning about the most serious problems that the user should pay attention to, even if he did not enable certain debugging channels. Therefore, output redirection should be turned off simply by running it in the console, regardless of the presence of -d/--debug.

The next comment will be even longer about the context and what problem the redirect was trying to solve and how to improve the solution...

@victoryforce
Copy link
Collaborator

Now for the big picture. The question is what problem the proposed change solves and whether the proposed solution will be better.

Redirecting output to the log file on Windows platform was not introduced for any technical reasons, not because output to the terminal had any problems. The reason is not the difference of platforms, but the difference of typical users. A characteristic feature of the Linux user base is that interaction with the command line does not frighten the absolute majority of them. Even if the user is not nerdy, the conscious choice to install Linux means that the user is at least ready to understand certain technical details.

Regarding macOS, this can be said to a lesser extent, but in practice the instructions for macOS users were usually quite clear for them.

The reality of the Windows platform is that it is the most common, and therefore among its users there are more those who have a fear of starting the command line and the need to type something there (and they will have to somehow copy that output, which many have never done in their lives). That is why the redirection of output to a file was initially made.

Logging the entire output of each program run to a log file eliminates the need to ask the user to run the program from the command line to get output for at least the most serious problems that are logged even without specifying debug arguments when the program is started. Of course, this will not help if such arguments are still necessary to diagnose problems, but at least it will help in some cases.

It turns out that the current implementation of output logging to a file is also not ideal. Existing problems:

  • The log is somewhere deep in the tree of the apps working directories (AppData), where the user never looks, so it is impossible to come across it without knowing the exact location
  • The program interface does not contain any link to log file or the ability to open it and copy its contents
  • The log file is located under a directory that is hidden, so with the default settings, it cannot be accessed by navigating in File Explorer. It is necessary to either teach the user to change the Windows setting to "show hidden files", or explain that the path they were told must be entered in the address field.

In fact, the correct solution to the problem of a complicated debugging process would be to modify the program interface so that the user can do everything without going to the command line and/or searching for a log file. This should include:

  • Ability to set debugging channels (i.e. parameters of the -d/--debug command line argument) in the dedicated tab of global program settings
  • Ability to open the log file from the program interface (it would be logical to place link to it in the same settings tab)
  • Placing the log in a more visible and easily accessible location, for example under the Documents directory.

There is nothing wrong with implementing both points 2 and 3, although even implementing one of them would be sufficient in principle.

@wpferguson
Copy link
Member

I would vote for point 3, putting the log file in the documents directory.

Opening the log file from inside the program is probably not much use. As you said, windows users aren't usually technically inclined so we'd end up having to explain what they are looking at. Better to just have the log file accessible so that they can attach it.

@jsmucr
Copy link
Author

jsmucr commented Aug 14, 2024 via email

@victoryforce
Copy link
Collaborator

My original concern was tailing the log, and figuring out what went wrong the very moment a failure occurs. The same way I'd do it in Linux.

You can tail the log very easily in Windows even without installing additional utilities:

  • Run PowerShell
  • In its window, type cat .\Documents\darktable-log.txt -Wait -Tail 30

As you can see, there is no need to modify the behavior of darktable to achieve this.

P.S. -Tail <number> part shows the last <number> lines of the file instead of full content.

@jsmucr
Copy link
Author

jsmucr commented Sep 22, 2024

no need to modify the behavior

I prefer to use the word "improve", or rather unify the behavior on Linux and Windows.
The PS' cat command is a good example of how it ends when one reinvents the wheel. 🙂

@victoryforce
Copy link
Collaborator

no need to modify the behavior

I prefer to use the word "improve",

I would replace the word "improve" here with more appropriate terms: feature creep, bloating.

or rather unify the behavior on Linux and Windows.

Above, I explained at length why it is better to understand the differences of users on different platforms, instead of unifying the behavior on Linux and Windows

The PS' cat command is a good example of how it ends when one reinvents the wheel. 🙂

PowerShell's cat command is just an alias for Get-Content command, added to make it easier for Linux users to remember.

@jsmucr
Copy link
Author

jsmucr commented Sep 23, 2024

Alright. Then tell me how Windows users report warnings based in Lensfun or GTK, none of which use the DT's logging system.

And as for cat (and curl, for example), I don't see how it helps to name a command the GNU way if it does something completely different, or in a very different way.

@victoryforce
Copy link
Collaborator

Alright. Then tell me how Windows users report warnings based in Lensfun or GTK, none of which use the DT's logging system.

Windows users report these warnings because they (warnings, not users) end up in the log file as warnings are output to stderr, which is redirected to the log file.

And as for cat (and curl, for example), I don't see how it helps to name a command the GNU way if it does something completely different, or in a very different way.

I don't understand why we are discussing this at all. Oh well, I could not mention the cat alias at all by specifying Get-Content as a command.

@jsmucr
Copy link
Author

jsmucr commented Sep 23, 2024

Windows users report these warnings because they (warnings, not users) end up in the log file as warnings are output to stderr, which is redirected to the log file.

Ah, okay, my bad. I caused this myself by using

darktable\bin\darktable.exe -d all "--configdir" "darktable-data" > debug.out

instead of

darktable\bin\darktable.exe -d all "--configdir" "darktable-data" > debug.out 2>&1

Could at least the log timestamp be altered to a date + time format instead of time from start, which makes it harder to review logs later?

Currently on Windows, DT redirects all output to

    %LOCALAPPDATA%\Microsoft\Windows\INetCache\darktable\darktable-log.txt

This commit overrides this behavior for Windows Terminal which is the default
terminal app since Windows 11, assuming that the user runs DT in a terminal for
debugging reasons.

It is however still perfectly possible to redirect DT output using the standard
redirection operator:

    darktable\bin\darktable.exe -d lua > darktable.log 2>&1
@victoryforce
Copy link
Collaborator

Could at least the log timestamp be altered to a date + time format instead of time from start, which makes it harder to review logs later?

You can raise the issue with a proposal as a feature request (providing, of course, an explanation of what it will improve) and the developers will respond.

Although I personally do not understand why this might be necessary. We now have the time since launch for each log entry and this makes it quite easy to see the duration of certain processes. Why would anyone need to know what time the clock on the wall shows at these moments?

@jsmucr
Copy link
Author

jsmucr commented Sep 24, 2024

Why would anyone need to know what time the clock on the wall shows at these moments

Because figuring out duration isn't the only purpose of logging, is it?
I personally would appreciate it when I work with an unstable version, debug set on all, and something unexpected happens. I look at the clock, safely finish my work (if possible), wait for DT to turn off (because this can take up to several minutes in such cases -- that's why I run DT from a script, so I know when it finally terminates), and then -- with the help of the timestamp -- go look for an error in the log file. Which is necessarily obfuscated by thousands of other reports, unless I know which part of DT causes the issue.
Now, since I'd have to tail the log file in an additional terminal window (because we don't want the code to get bloated, duh) to see the error immediately as it happens (and I'd hate to do that "just in case"), the timestamp may be the only thing that helps me.
So yeah, I'll go create another issue, but as you can see, from my perspective, these two are closely related.

@TurboGit TurboGit added the scope: windows support windows related issues and PR label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: redesign current features to rewrite scope: windows support windows related issues and PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants