natinusala / borealis

Hardware accelerated, controller and TV oriented UI library for PC and Nintendo Switch (libnx)
Apache License 2.0
260 stars 83 forks source link

Skip MSYS2 path conversion in the example makefile #34

Closed p-sam closed 4 years ago

p-sam commented 4 years ago

https://www.msys2.org/wiki/Porting/

This PR is supposed to mitigate the following error people might encounter when building the borealis example for the Switch.

error: missing terminating " character

This is due to the fact programs under MSYS2 will try to convert path in arguments and env variables matching the pattern XXX:/ and convert them to a posix path. We skip this behavior for defines by telling MSYS2 to skip arguments starting with -D

meganukebmp commented 4 years ago

Converting all -Ds might be a bit too broad. It's possible that people might use the example Makefile for their own projects or even the example itself changing, with defines that actually rely on windows style paths.

Additionally given that this is a platform specific thing, I personally don think it needs to be in the Makefile. Initially I assumed it was just a missing backslash or bad formatting. But this is a MSYS2 issue and is not up to this project to solve.

meganukebmp commented 4 years ago

I made a more general informative change to the readme instead. https://github.com/meganukebmp/borealis/commit/81ed316de4d21650473dce62d46f2ad73b7cd7b3 This requires a person building with MSYS2 to manually export, but it would bring this problem to their attention so when and if it does come up again they'll be able to resolve it themselves

p-sam commented 4 years ago

I suggested only "-DDBOREALIS_RESOURCES" first, but we agreed to expand to all defines, since for example sys-clk also use borealis, and has another define for sys-clk specific assets, so it makes more sense to show it in the example, else you're implicitly pushing others to adding each define manually, even though all are meant to be values for building on the Switch target.

While I agree with you that it's not a problem tied to borealis, without mitigating the problem on our side it won't compile at all, and a good chunk of users on windows will come from the devkitpro msys installer. You're asking users to use a different command on that environment, where we can have a ready makefile that seems to work on all platforms.

meganukebmp commented 4 years ago

While I agree that simply having it there would be best for most new users, I dont generally like the clutter bolted on top for something not everyone is going to use. In the end of the day it probably wont hurt anything, but call it programmer OCD. If it's not our problem we tell the user how to fix it an leave it at that. Alternatively having it all inside an OS check would be far cleaner. In the end this is all nitpicking. It's not my project after all.

natinusala commented 4 years ago

I see multiple things here.

  1. Having to write the long export every time we want to make from MSYS2 is a pain in the butt and I don't want that - borealis is supposed to work "out of the box" and be easy to integrate in a new project
  2. It's a platform-specific problem, yes, but that platform is so popular among Switch devs that we can't simply ignore the issue
  3. I would actually argue that translating paths inside of -D is a bug from MSYS2. It's supposed to be a fixed string literal passed to the compiler. So to me all defines should be excluded from that translation system
  4. I don't expect or want people to use the example Makefile for anything else than writing a homebrew with borealis

In the end that change is the cleanest in the way that it fixes the issue and it doesn't break other platforms.