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

Fix Windows console setup so it works in an MSYS2 window. #882

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

acolwell
Copy link
Collaborator

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

PR Description

What type of PR is this? (Check one of the boxes below)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
    • I have updated the documentation accordingly

What does this pull request do?

This change adds checks to see if a console is already attached and avoids reopening stdout & stderr if so. This allows console logging to work properly in the typical MSYS2 bash window. For some reason the freopen() calls in the old code cause console printing to stop working if a console was already attached (e.g. the MSYS2 bash window case).

Have you tested your changes (if applicable)? If so, how?

Yes. I've tested this by running Natron from a MSYS2 bash window, a normal Windows Command Prompt window, and launching Natron from the start menu with the "enable console windows" preference set. All of them now show the same logging info.

Futher details of this pull request

[Your answer]

@devernay devernay requested a review from rodlie May 10, 2023 22:56
@rodlie
Copy link
Contributor

rodlie commented May 18, 2023

Sorry for the delay, I assume this does not break the "native" terminal in Windows (cmd/powershell)?

Will test on Windows a bit later today.

@acolwell
Copy link
Collaborator Author

acolwell commented May 18, 2023

Sorry for the delay, I assume this does not break the "native" terminal in Windows (cmd/powershell)?

Will test on Windows a bit later today.

Yes. I tested it with cmd and it works. That basically runs the path where the handles are not set and it has to use AttachConsole() to get them set. It seems like MSYS2 is the main case that actually sets the handles up front. The cmd.exe console uses the AttachConsole() path. Launching from File Explorer or the start menu trigger the AllocConsole() path.

@rodlie
Copy link
Contributor

rodlie commented May 19, 2023

Great.

Some more delays on my part I'm afraid. I use msys64-20220603 to do release builds, and it's no longer compatible with the msys changes done in Natron lately, so I will need to do a clean install of latest msys based on your pull #879.

Will do so after work today, finally got some spare time this weekend that I can dedicate to Natron.

@acolwell
Copy link
Collaborator Author

Friendly ping. :)

@rodlie
Copy link
Contributor

rodlie commented Jul 3, 2023

I'm really sorry, I totally forgot this. Will test after work today (I need to test a bunch of Windows stuff anyway).

This change adds checks to see if a console is already attached
and avoids reopening stdout & stderr if so. This allows console
logging to work properly in the typical MSYS2 bash window. For
some reason the freopen() calls in the old code cause console
printing to stop working.
@acolwell
Copy link
Collaborator Author

acolwell commented Sep 5, 2023

Friendly ping 2. FWIW I've been using this for months so I'm pretty sure it is safe.

@rodlie
Copy link
Contributor

rodlie commented Sep 5, 2023

I'm sorry for not doing anything, too much other stuff in the way. I also don't have a free Windows PC to test Natron on anymore (can be fixed, but will probably take a week or two).

So, I trust that you have tested this and will approve.

@acolwell acolwell merged commit b8496b3 into NatronGitHub:RB-2.5 Sep 5, 2023
2 checks passed
@acolwell acolwell deleted the fix_windows_console_setup branch September 5, 2023 15:02
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.

2 participants