Windows compatibility fixes (spaces, stubs, includes) - #86
Conversation
|
I compiled and tested it using this batch file: @echo.
@rd /q /s build
@mkdir build && cd build
@cmake -G "MinGW Makefiles" -DCMAKE_BUILD_TYPE=Release -DCMAKE_EXE_LINKER_FLAGS="-s -static -static-libgcc -static-libstdc++" -DIconv_LIBRARY="C:/msys64/ucrt64/lib/libiconv.a" -DOGG_LIBRARIES="C:/msys64/ucrt64/lib/libogg.a" ..
@make -j4
ntldd opustags.exeI'm using MSYS2 UCRT64, it compiles into a static binary that does not require DLLs other than kernel32.dll and well the UCRT windows dlls which are already there in Windows 10 and Windows 11 |
… missing <cstdint>
fmang
left a comment
fmang
left a comment
There was a problem hiding this comment.
Thank you for the pull request!
It is too big for me to review and merge it easily, so I’d be grateful if you split it. For example:
- the part with byte endianness and cstdint,
- the file manipulation operations,
- the compatibility functions.
| #endif | ||
|
|
||
| #if defined(_WIN32) || defined(__MINGW32__) | ||
| #include <cstdint> |
There was a problem hiding this comment.
Rather than include cstdint in the .h and in every .cc, including it in opustags.h only is enough since all the .cc include it.
|
|
||
| // Applying the new tags. | ||
| tags_file = fopen(tags_path.c_str(), "re"); | ||
| tags_file = fopen(tags_path.c_str(), "rb"); |
| inline uint32_t le32toh(uint32_t x) { return x; } | ||
| inline uint32_t htobe32(uint32_t x) { | ||
| return ((x & 0xffu) << 24) | ((x & 0xff00u) << 8) | | ||
| ((x & 0xff0000u) >> 8) | ((x >> 24) & 0xffu); |
There was a problem hiding this comment.
Are the & 0xffu necessary?
Also, doesn’t Windows already provide such a function, like htonl?
| #include <unistd.h> | ||
| #include <cstdint> | ||
|
|
||
| #ifndef _WIN32 |
There was a problem hiding this comment.
I find it curious to mix #ifndef and #if defined. Why not use an #else instead?
| { | ||
| #ifdef _WIN32 | ||
| // Windows cmd.exe: double quotes protect spaces. | ||
| return "\"" + std::string(word) + "\""; |
There was a problem hiding this comment.
If word contains a double quote, that’s an open door to serious shell injection vulnerabilities.
| #include <algorithm> | ||
| #include <cstdint> | ||
|
|
||
| #if defined(_WIN32) || defined(__MINGW32__) |
There was a problem hiding this comment.
Why check both _WIN32 and __MINGW32__. Isn’t _WIN32 enough?
| return; | ||
| file.reset(); | ||
| copy_permissions(final_name.c_str(), temporary_name.c_str()); | ||
| #if defined(_WIN32) || defined(__MINGW32__) |
There was a problem hiding this comment.
You could put the call to copy_permissions in an #else here instead of defining a blank function.
| std::string command = std::string(editor) + " -- " + shell_escape(path); | ||
| #endif | ||
|
|
||
| int status = system(command.c_str()); |
There was a problem hiding this comment.
If this is the only line in common between Windows and Unix, you might as well make on big #if instead of two halves.
| #define strncasecmp _strnicmp | ||
|
|
||
| #ifndef getdelim | ||
| static inline ssize_t getdelim(char** lineptr, size_t* n, int delim, FILE* stream) { |
There was a problem hiding this comment.
This function is huge for an inline function. You could move the definition into system.cc and add the declaration into opustags.h instead so that it is built and linked like a regular function.
By the way, where does the implementation come from?
There was a problem hiding this comment.
I wrote the getdelim (and mkstemps) implementations myself to get the Windows build working. I looked at a few public versions for reference while writing it, but this is my own code. Happy to move both functions out of the header into system.cc (and just declare them in the header or in opustags.h) if you prefer that style. I can also clean them up further if you’d like.
There was a problem hiding this comment.
Thank you. Since C++ is loaded with footguns so I’ll make sure to review it attentively, unless we find a battle-tested alternative.
Maybe we could replace getdelim with std::getline, provided the code does not rely too much on stdio. Regarding mkstemps, Windows seems to have _mktemp that could do some of the work.
There was a problem hiding this comment.
Nice, I will look into _mktemp, Windows seems very messy. I'll look into all this more thoroughly on Saturday including using getline instead of getdelim. My PR was pretty much out of excitement that I actually got it to compile and work on Windows. I've been wanting / needing opustags for Windows for a while.
|
|
||
| #define strncasecmp _strnicmp | ||
|
|
||
| #ifndef getdelim |
There was a problem hiding this comment.
What is this for? Is it possible that getdelim is sometimes defined as a macro, sometimes not, but never defined as a function?
There was a problem hiding this comment.
That is so it uses the windows equivalent of strncasecmp, I'll work on all these changes and make it suitable. I have a busy week coming up and start work in 2 hours, I'l try to clean all this up and explain on Saturday.
Sorry for the lack of explanation on these changes and having to wait until Saturday.
There was a problem hiding this comment.
I think I misunderstood your question. I added the #ifndef getdelim because I wanted to prevent a duplicate definition if the platform already supplied getdelim(). I realize now that this only checks for a preprocessor macro, not whether the function is already provided.
There was a problem hiding this comment.
Exactly. If the _WIN32 macro check is not enough, we should rely on CMake because that’s precisely what it’s for. See CheckFunctionExists.
2 participants
This patch makes opustags compile and run correctly on Windows (MSYS2 UCRT64 / cmd.exe):
-einteractive editing win32 compatibility with filenames containing spaces by using double quotes inshell_escapeon Windows.<cstdint>includes inbase64.cc,ogg.cc,opus.cc.strncasecmp,getdelim,mkstemps) intowin32_compat.h.sys/wait.hconditional to avoid Windows build error.htole32, etc.) for Windows inopustags.h.