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

C++17 temp_directory_path() broken on Windows #2203

Open
time-killer-games opened this issue Mar 25, 2021 · 14 comments
Open

C++17 temp_directory_path() broken on Windows #2203

time-killer-games opened this issue Mar 25, 2021 · 14 comments

Comments

@time-killer-games
Copy link
Contributor

time-killer-games commented Mar 25, 2021

I can tell Josh and fundies have a fetish for having broken c++17 features in enigma. emake broken.

std::ofstream egmlog(fs::temp_directory_path().string() + "/enigma_libegm.log", std::ofstream::out);

Also please use path.u8string() instead of path.string() to avoid creating more tech debt for when this code eventually works on english-only machines.

If you guys ever asked me to do code review, pitiful mistakes like these wouldn't be merged, because I actively use and test all our desktop platforms and value them as equals and not with a Linux bias/fetish.

@time-killer-games time-killer-games changed the title C++17 get_temp_directory broken on Windows C++17 temp_directory_path() broken on Windows Mar 25, 2021
@fundies
Copy link
Contributor

fundies commented Mar 25, 2021

broken how? Is it crashing? Always or only if you pass a gmk? Also, you can review any PR you want they're all public

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Mar 25, 2021

Running emake complains with an error of temp_directory_path failing because the c++17 devs never got it working on Windows. Last I checked you already knew this because we talked about it before and you confirmed it with Robert or whoever so it needs no further explanation

Pretty much prevents using emake completely.

@fundies
Copy link
Contributor

fundies commented Mar 25, 2021

It's just a log file you shouldn't need it to work though

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Mar 25, 2021

Yes but the fact it's even called prevents running emake. I believe this is probably due the fact you didn't use the no except version but I'll double check the docs

Edit:

Oddly enough there is no "no except" version in the docs but that's probably a problem with the docs being written correctly. It throws an exception and in my memory passing an error code argument will allow it to fail silently.

@fundies
Copy link
Contributor

fundies commented Mar 25, 2021

Also afaict the CI is running empty games on windows fine. If it's just running projects that broke you can add a test for that pretty easy but I'd rather someone port the test harness to windows

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Mar 25, 2021

It could be I may need a package update and they may have fixed it. I'll install master to verify. I was too quick to delete my install.

Of course I could just make test program as well outside of enigma

@time-killer-games
Copy link
Contributor Author

#include <iostream>
#include <filesystem>

int main() {
  std::cout << std::filesystem::temp_directory_path().string() << std::endl;
  return 0;
}

Capture

@time-killer-games
Copy link
Contributor Author

where as:

#include <iostream>
#include <filesystem>
#include <system_error>

int main() {
  std::error_code ec;
  std::cout << std::filesystem::temp_directory_path(ec).string() << std::endl;
  return 0;
}

Capture

This is why CI will not ever be as reliable as doing it yourself. The CI is a mere excuse to waste time writing CI instead of doing real tests.

@time-killer-games
Copy link
Contributor Author

The funny thing is, idk how the C++17 devs fuckered this one up, they claim it uses GetTempPath() on Windows underneath and that has always worked for me, I even use that as a workaround to this problem in my GM extensions.

@time-killer-games
Copy link
Contributor Author

And finally:

#include <iostream>
#include <windows.h>

int main() {
  wchar_t buffer[MAX_PATH + 1];
  if (GetTempPathW(MAX_PATH + 1, buffer)) {
    std::wcout << buffer << std::endl;
  }
  return 0;
}

Capture

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Mar 25, 2021

And would you just look at that:

Capture

At least we learned a valuable lesson today. UTF-8 can still technically be supported if we add but two characters, "u8" in every fucking place you guys forgot to. I learned something too, update before whining shite broke.

@time-killer-games
Copy link
Contributor Author

well it's still broken even after the update. tf...

#include <iostream>
#include <filesystem>

int main() {
  std::cout << std::filesystem::temp_directory_path().string() << std::endl;
  return 0;
}

Capture

@fundies
Copy link
Contributor

fundies commented Mar 25, 2021

I would talk to mingw people about it. However, feel free to implement your GetTempPathW in shared/filesystem and replace my call with your routine

@time-killer-games
Copy link
Contributor Author

time-killer-games commented Mar 26, 2021

Unfortunately that's a bit more involved than I have time for right now, because a lot of things like this require moving our widen() and narrow() functions to shared, which I've tried doing in a recent pr I ended up losing patience and closing it because of how many things that get broken by moving those which needed to be resolved.

I'll do it when I'm feeling less depressed some day. lel

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

Successfully merging a pull request may close this issue.

2 participants