noexcept std::filesystem

Reporter: time-killer-games  |  Status: open  |  Last Modified: September 12, 2020, 02:43:52 PM

fuck joo bitch !!!!
time-killer-games  
This code doesn't do anything different from what is already in master with the POSIX and Win32 only this uses the std::filesystem. I don't understand why we need security measures for std::filesysttem but not for POSIX/Win32 file manipulation because it is just using a different api to accomplish the same exact thing.

I'm happy to add security measures but that was not the point of this pr. It was to get rid of platform-specific POSIX and Win32 code. I'd like to make that a different pull request, and not really make that pull request at all until we add a proper means to turn off the sandbox.

This code isn't anymore dangerous than what we already have in the engine right now, not even by 1%. I even went ahead and used fs::remove instead of fs::remove_all so only empty directories can be deleted, which is what we have going on allowed in the engine already just like everything else.

fundies  

This code doesn't do anything different from what is already in master with the POSIX and Win32 only this uses the std::filesystem. I don't understand why we need security measures for std::filesysttem but not for POSIX/Win32 file manipulation because it is just using a different api to accomplish the same exact thing.

I'm happy to add security measures but that was not the point of this pr. It was to get rid of platform-specific POSIX and Win32 code. I'd like to make that a different pull request, and not really make that pull request at all until we add a proper means to turn off the sandbox.

This code isn't anymore dangerous than what we already have in the engine right now, not even by 1%. I even went ahead and used fs::remove instead of fs::remove_all so only empty directories can be deleted, which is what we have going on allowed in the engine already just like everything else.

I wasn't arguing that yours is any more dangerous, my point is that while you're rewriting it all is a good time to secure these as well as to add some tests.

time-killer-games  

lol, ok good, I'm glad that's the case. You had me worried for a minute there. My only opposition with adding the security measures right now is that it will break existing games. If we waited until the sandbox could be turned off, then people could turn the sandbox on exclusively for their bug report reproducible examples, while not messing up their complete games, which some of them are going to be ported over from GameMaker, and having a sandbox not optional even for a temporary time won't be very convenient. People will spend time trying to make their project work with the enigma sandbox just to find out its getting removed again with a tickbox in the IDE. For these reasons given, I think it would be better if we waited on that.

If I'm going to add tests, I'll need to chat in realtime on hangouts with Josh about that so he can tell me the ins and outs for that. I do think it is better I learn to do tests sooner than keep on putting that off every day. I 100% agree I should do that at this point.

time-killer-games  

@fundies i removed directory_delete(). we initially added that because GameMaker used to not have a function for deleting directories so we made our own. Then yoyo added it in Studio and called it directory_destroy(). Should I make a function wrapping directory_destroy() so that projects using directorty_delete(() won't break or should I keep it how it is removing directory_delete in favor of directory_destroy() for a GM compatible function only.
fundies  

lol, ok good, I'm glad that's the case. You had me worried for a minute there. My only opposition with adding the security measures right now is that it will break existing games. If we waited until the sandbox could be turned off, then people could turn the sandbox on exclusively for their bug report reproducible examples, while not messing up their complete games, which some of them are going to be ported over from GameMaker, and having a sandbox not optional even for a temporary time won't be very convenient. People will spend time trying to make their project work with the enigma sandbox just to find out its getting removed again with a tickbox in the IDE. For these reasons given, I think it would be better if we waited on that.

If I'm going to add tests, I'll need to chat in realtime on hangouts with Josh about that so he can tell me the ins and outs for that. I do think it is better I learn to do tests sooner than keep on putting that off every day. I 100% agree I should do that at this point.

You can add security measures without breaking games. I suggest something like this:

std::map<std::filesystem::path, bool> allowed paths = {"~/.enigma" };

bool fs_access_allowed(fs::path) {
  if (!map.contains(path)) { result = show_dialog("you allow access to derp?") map[path] = result; }
  if (map[path] == false) { DEBUG_MESSAGE("permission denied"); }
  return map[path];
}

As for tests, If you look at the ones we have and know how to run emake it should be rather easy to write some.

fundies  

lol, ok good, I'm glad that's the case. You had me worried for a minute there. My only opposition with adding the security measures right now is that it will break existing games. If we waited until the sandbox could be turned off, then people could turn the sandbox on exclusively for their bug report reproducible examples, while not messing up their complete games, which some of them are going to be ported over from GameMaker, and having a sandbox not optional even for a temporary time won't be very convenient. People will spend time trying to make their project work with the enigma sandbox just to find out its getting removed again with a tickbox in the IDE. For these reasons given, I think it would be better if we waited on that.
If I'm going to add tests, I'll need to chat in realtime on hangouts with Josh about that so he can tell me the ins and outs for that. I do think it is better I learn to do tests sooner than keep on putting that off every day. I 100% agree I should do that at this point.

You can add security measures without breaking games. I suggest something like this:

std::map<std::filesystem::path, boolallowed paths = {"~/.enigma" };

bool fs_access_allowed(fs::path) {
  if (!map.contains(path)) { result = show_dialog("you allow access to derp?") map[path] = result; }
  if (map[path] == false) { DEBUG_MESSAGE("permission denied"); }
  return map[path];
}

This would prompt you once per run to access specific files but all games would still work

As for tests, If you look at the ones we have and know how to run emake it should be rather easy to write some.

time-killer-games  

By breaking games I do include the fact this will ruin user experience for finished games. You are free to do this in your own pr after Josh merges this one. I don't want to be responsible for people complaining, and I'd rather them blame the person who actually insisted on this idea, not me.

@JoshDreamland this one is ready when you are.

fundies  

I will review / merge this if you write a test suite. Or you can wait on josh if you prefer
time-killer-games  

Yeah I actually forgot I said I would do that. I have a look at our current CI tests right now and see if I can make enough sense of it to get it done.
RobertBColton  

@time-killer-games ENIGMA should not do instance creation ordering or provide that as a feature, it's arguably a bad thing. You should not design your game in a way that instances depend on other instances being created first. That creates a hard dependence which can make it more difficult to refactor your project down the road. I don't really know why YoYoGames went overboard on that one. It's not even possible in other engines, like Unity3D where the start execution order is non-deterministic, and yet those engines are just fine. You want to do loose coupling of components in your game.
time-killer-games  

@RobertBColton what game are you referring to that does this exactly? Last I checked none of my games are dependent on that. If they were, they wouldn't look right in Studio.

Although I noticed you were working on object layering recently. If you are having trouble doing that correctly that's a pretty lame excuse to drop instance ordering if you ask me. It is just laziness is all it amounts to.

In fact, I'd rather go the other way around and drop layering, at least until we can find someone who is willing to do it without an approach that will likely break a lot of games from older versions of GM.

"No other engines are doing it" is not an excuse to not do it; it is a reason to do it. It gives us an advantage over other engines by providing more functionality and ease of use.

You don't have to listen to me, I'm just giving you my opinion. This one isn't my call.

RobertBColton  

@time-killer-games Hey woah, you're going way overboard 🏴‍☠️

No, the instance creation ordering would actually be the simplest thing to accomplish. The layering is actually going great because we have some innovation over GMS which just plagiarized Tiled anyway. I wasn't referring to any of your specific games, just happened to run into creation ordering again examining the new room editor behavior. I've been staunchly opposed to it for a while, for good reason, and I want to warn everybody why it's bad. I'm not saying it can't be added for compatibility, but that it should be hidden away. It's almost as bad as using instance ids rather than names in your code, even though both are still supported.

time-killer-games  

@RobertBColton thank you for the clarification lel
time-killer-games  

@fundies i added a filesystem simple test now does the CI detect it automatically or how do I make it recognize it now that I've written it?
fundies  

@fundies i added a filesystem simple test now does the CI detect it automatically or how do I make it recognize it now that I've written it?

Yes it will run "simple tests" automatically. You can see in the log of the azure job:

[ FAILED ] SimpleTests/SimpleTestHarness.SimpleTestRunner/6, where GetParam() = "CommandLine/testing/SimpleTests/file_input_output.sog"
[ FAILED ] SimpleTests/SimpleTestHarness.SimpleTestRunner/7, where GetParam() = "CommandLine/testing/SimpleTests/filesystem.gmx"

You broke one test and are failing your own.

time-killer-games  

@fundies i added a filesystem simple test now does the CI detect it automatically or how do I make it recognize it now that I've written it?

Yes it will run "simple tests" automatically. You can see in the log of the azure job:

[ FAILED ] SimpleTests/SimpleTestHarness.SimpleTestRunner/6, where GetParam() = "CommandLine/testing/SimpleTests/file_input_output.sog"
[ FAILED ] SimpleTests/SimpleTestHarness.SimpleTestRunner/7, where GetParam() = "CommandLine/testing/SimpleTests/filesystem.gmx"

You broke one test and are failing your own.

Where are you gettting that information from? I don't see it under travis.

Please sign in to post comments, or you can view this issue on GitHub.