search

OutpostUniverse / op2ext

Outpost 2 extension module loader
1 stars 0 forks source link

Make locate vol files use an absoluate directory #230

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

I would recommend reviewing and merging this branch before #227.

I think it will ensure directories provided by ini modules will be correct.

DanRStevens commented 5 years ago

The fs::relative method is not available in the <experimental/filesystem> library shipped with Mingw. I actually just discovered that earlier today, and commented about it in issue #93.

Brett208 commented 5 years ago

I've never used filesystem::relative before. It worked like I was hoping on MSVC. Not sure, but maybe, mingw just doesn't actually support all of filesystem yet?

DanRStevens commented 5 years ago

With that said though, I have been wanting to update this method for a while now. It can probably even be moved to the static library project. Though I think I would like to remove the exception handling and let the caller deal with any exceptions.

Brett208 commented 5 years ago

I'm taking a break if you want to play with this.

I'd propose writing a function like IsDirectory that mimicks fs::relative. That way when mingw pulls in full support for filesystem, we can delete the function and just use fs::relative.

I was assuming that mingw was no longer using an experimental filesystem for some reason...

Brett208 commented 5 years ago

I wanted to write some basic unit tests for VolList since I added a new public function to it. However, we need to get the mock test logger branches figured out first to be actually productive, so I just left it as a stub.

Maybe better to write an issue to provide some VolList tests.

The fs::relative not on mingw is still an issue. If recreating fs::relative seems difficult, we could just implement an alternate solution and add a comment saying to switch to relative when mingw catches up with C++17 standard.

-Brett

Brett208 commented 5 years ago

I'm okay with using the word Find instead of Locate (accomplished in separate branch). Work on this branch should be on hold until PR #232 is finished due to how intertwined they are.

DanRStevens commented 5 years ago

We may have broken console module loading. I tried testing with /loadmod cm1. It ran without error, but the color module clearly wasn't loaded. I'll look more into this shortly.

Brett208 commented 5 years ago

I just checked, and this branch is properly loading a console module and an ini module on my computer. I did notice that loose resources stored in a console module directory were not being recognized. It appears directories must be passed as absolute, which I updated in a commit.

@DanRStevens, could you double check and see if you can load modules on your build? If so, I'm okay with merging unless you want to write up some unit tests for the new Filesystem function beforehand.

-Brett