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 pipe output on windows #852

Merged
merged 4 commits into from
May 5, 2023
Merged

Conversation

vrunk11
Copy link
Contributor

@vrunk11 vrunk11 commented Apr 21, 2023

Fix pipe output on Windows for all tool by setting output mode to binary instead of the default mode (text mode)

vrunk11 added 2 commits April 21, 2023 18:09
Add .theia/ folder in .gitignore (ide based on vscode)
Fix pipe output on windows for all tool by setting output mode to binary instead of the default mode (text mode)
@vrunk11
Copy link
Contributor Author

vrunk11 commented Apr 21, 2023

It compiles on my setup using mingw64 but seem to fail here

@oyvindln
Copy link
Contributor

The includes also needs a #ifdef _WIN32 || _WIN64 guard, those are windows-speficic.

IMO we should also put this in a setup function in library/tbc/something that can be called rather than copy pasting the same code in each file.

add pre-processor instruction to fix building with linux
@vrunk11
Copy link
Contributor Author

vrunk11 commented Apr 21, 2023

The includes also needs a #ifdef _WIN32 || _WIN64 guard, those are windows-speficic.

IMO we should also put this in a setup function in library/tbc/something that can be called rather than copy pasting the same code in each file.

I think we can't put it in a separate file cause the instruction need to be in the main()

@oyvindln
Copy link
Contributor

oyvindln commented Apr 21, 2023

I don't see why it has to, it doesn't say anything about that in the documentation. The two calls to the windows functions just have to be set before we pipe any data

Like make a set_binary_mode or whatever name functions that contains the

#ifdef _WIN32
   _setmode(_fileno(stdout), O_BINARY);
   _setmode(_fileno(stdin), O_BINARY);	
#endif

code and call that function at the beginning of each main instead of having those lines copied in each main. This makes it much cleaner.

@atsampson
Copy link
Collaborator

There's already some helper code in library/tbc/logging.cpp that gets used in the main function of all the tools. You could add a new file there, or (given it's a really short function and it's sort-of to do with log output) add it to logging.cpp.

I wondered if there was a way of doing this using Qt when stdout is opened, but no - looking at the Qt source, all of their tools have exactly the same hack in them!

(Mind the indentation too - it should match the existing code, with four-space indentation and no tabs.)

@vrunk11
Copy link
Contributor Author

vrunk11 commented Apr 22, 2023

ld-process-ac3 don't have the loggin.cpp included, so maybe we should include it ?

@oyvindln
Copy link
Contributor

I don't think they need to, they don't make use of Qt so would be preferable to not link to the qt specific stuff in e.g logging.cpp

Move setmode binary to logging.cpp
@vrunk11
Copy link
Contributor Author

vrunk11 commented Apr 23, 2023

it should be good now

@oyvindln
Copy link
Contributor

Looks good now I think other than ld-chroma-decoder/encoder can also use the function since it uses logging

@happycube happycube merged commit ccc8f54 into happycube:master May 5, 2023
@vrunk11 vrunk11 deleted the cmake-windows-edit branch September 22, 2024 06:23
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.

4 participants