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

libhb/gtk: do not modify stderr struct directly on mingw #5945

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

Conversation

marcosfrm
Copy link
Contributor

Breaks mingw build when using UCRT:

../libhb/hb.c: In function 'redirect_thread_func': ../libhb/hb.c:2314:11: error: 'FILE' {aka 'struct _iobuf'} has no member named '_file'
 2314 |     stderr->_file = pfd[1];
      |           ^~

See:

mingw-w64/mingw-w64@fe226dd

Implement the same approach used by the GTK UI code:

#if defined(_WIN32)
_dup2(pfd[1], _fileno(stderr));
#else
dup2(pfd[1], STDERR_FILENO);
#endif

Tested on:

  • Windows 10+ (via MinGW)
  • macOS 10.13+
  • Ubuntu Linux
  • Fedora 39 mingw cross-compile (with both ucrt and msvcrt)

@bradleysepos
Copy link
Contributor

Thanks for this. Where are you getting your mingw-w64 toolchain from, Fedora or the included build script, somewhere else?

@marcosfrm
Copy link
Contributor Author

Official Fedora packages: mingw64-gcc-c++ (prefix x86_64-w64-mingw32) and ucrt64-gcc-c++ (prefix x86_64-w64-mingw32ucrt) are enough. Required dependencies are installed automatically.

https://docs.fedoraproject.org/en-US/packaging-guidelines/MinGW/

@sr55
Copy link
Contributor

sr55 commented Apr 12, 2024

I'm afraid this doesn't work. We get no output on this in Windows.

@marcosfrm
Copy link
Contributor Author

Sorry for not testing it. I was only compiling the CLI version and assumed it was working being there in the GTK code.

Found this: https://stackoverflow.com/a/25927081

This

    freopen("NUL", "w", stderr);
    _dup2(pfd[1], _fileno(stderr));

works on both ucrt and msvcrt here. @sr55 please give it a try.

@marcosfrm marcosfrm force-pushed the libhb-dup2 branch 2 times, most recently from 7aec87e to 8376af5 Compare April 13, 2024 10:59
@marcosfrm marcosfrm changed the title libhb: do not modify stderr struct directly on mingw libhb/gtk: do not modify stderr struct directly on mingw Apr 13, 2024
@marcosfrm
Copy link
Contributor Author

(I did not test the GTK part)

@jstebbins
Copy link
Contributor

The mingw port of the gtk gui rarely gets tested as far as I know. I did the port just for kicks and then pretty much set it aside. It's probably full of bit-rot.

@marcosfrm
Copy link
Contributor Author

With UCRT, all logging before the redirection is lost ("Compile-time hardening features are enabled" for example). It does not occur with MSVCRT somehow.

@galad87
Copy link
Contributor

galad87 commented Apr 28, 2024

Looks good, tested on Windows and it looks fine.

stderr->_file = pfd[1];
// Non-console windows apps do not have a stderr->_file
// assigned properly
(void) freopen("NUL", "w", stderr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, does this leave a file named "NUL" somewhere that needs to be cleaned up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not. NUL is the old DOS device which Windows will convert behind the scenes to \Device\Null, its /dev/null equivalent.

windows-nul

Copy link
Contributor

@jstebbins jstebbins left a comment

Choose a reason for hiding this comment

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

LGTM

But since this affects Windows, I'll let @sr55 make the final decision to merge

@sr55
Copy link
Contributor

sr55 commented May 14, 2024

Sorry for the delayed response.

Looks like this is still causing a regression on Windows. We are losing some (not all) log messages now, rather than them being completely missing. As if a buffer is not being flushed correctly.

Before then After
image

Note, the After on the right is missing a bunch of logging that only later appears once the encode finalises.

Breaks mingw build when using UCRT:

../libhb/hb.c: In function 'redirect_thread_func':
../libhb/hb.c:2314:11: error: 'FILE' {aka 'struct _iobuf'} has no member named '_file'
 2314 |     stderr->_file = pfd[1];
      |           ^~

See:

mingw-w64/mingw-w64@fe226dd
@marcosfrm
Copy link
Contributor Author

Let's try _IONBF, set in the GTK code already.

@marcosfrm
Copy link
Contributor Author

Note: setvbuf(stderr, NULL, _IONBF, 0); called on non-Windows too.

@sr55
Copy link
Contributor

sr55 commented May 15, 2024

That appears to behave better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants