[fixed] CFileSystem::getAbsolutePath()

[pc0de]

On Linux passing a valid relative path (i.e. "../x") returns what was passed. This small update appears to fix the problem.

Code: Select all

Index: source/Irrlicht/CFileSystem.cpp
===================================================================
--- source/Irrlicht/CFileSystem.cpp	(revision 2941)
+++ source/Irrlicht/CFileSystem.cpp	(working copy)
@@ -457,7 +457,7 @@
 	c8 fpath[4096];
 	fpath[0]=0;
 	p = realpath(filename.c_str(), fpath);
-	if (!p)
+	if (p)
 	{
 		// content in fpath is undefined at this point
 		if (!fpath[0]) // seems like fpath wasn't altered

[CuteAlien]

According to documentation realpath should return a non-null value when it did succeed in creating an absolute path. So the old code looks correct. I also tried to reproduce it with the following code on linux, but I got the correct results in my case. Do you have a test-case to reproduce the problem?

Code: Select all

#include <irrlicht.h>
#include <iostream>

using namespace irr;
using namespace core;
using namespace io;

#ifdef _IRR_WINDOWS_
#pragma comment(lib, "Irrlicht.lib")
#endif

int main()
{
	video::E_DRIVER_TYPE driverType = video::EDT_NULL;
	IrrlichtDevice * device = createDevice(driverType, core::dimension2d<u32>(640, 480));
	if (device == 0)
		return 1; // could not create selected driver.

	io::IFileSystem * fs = device->getFileSystem();

	path absPathInvalid = fs->getAbsolutePath ("../nothere.txt");	// file does not exist
	path absPathValid = fs->getAbsolutePath ("../Makefile"); 		// file does exist
	path absPathValidFolder = fs->getAbsolutePath ("../michatest"); 		// folder does exist
		
	std::cout << "absPathInvalid: " << absPathInvalid.c_str() << "\nabsPathValid: " << absPathValid.c_str() << "\nabsPathValidFolder: " << absPathValidFolder.c_str() << std::endl;
		
	device->drop();

	return 0;
}

[pc0de]

Do you have a test-case to reproduce the problem?

Sure do:

Code: Select all

    nd = createDevice(EDT_NULL);

    fs = nd->getFileSystem();

    path apath = fs->getAbsolutePath ("../nothere.txt");   // file does not exist
    printf("../nothere.txt: %s\n", apath.c_str());

    apath = fs->getAbsolutePath("../data");
    printf("../data: %s\n", apath.c_str());

    apath = fs->getAbsolutePath("../data/");
    printf("../data/: %s\n", apath.c_str());

    
    nd->drop();

Produces the following output:

Code: Select all

../nothere.txt: /home/kmurray/gdev/tubras/nothere.txt
../data: ../data
../data/: ../data/

With the patch I posted above, the output is:

Code: Select all

../nothere.txt: ../nothere.txt
../data: /home/kmurray/gdev/tubras/data
../data/: /home/kmurray/gdev/tubras/data

In my case I'm running the test application from "/home/kmurray/gdev/tubras/bin/" and the "/home/kmurray/gdev/tubras/data/" folder exists.

According to documentation realpath should return a non-null value when it did succeed

That's why I thought it was odd that if realpath succeeds (if(!p) check fails), then it returns what was passed as input "filename" instead of "fpath".

[CuteAlien]

Ah, I see... this was fixed already in the svn trunk - which I had checked. It's still wrong in the 1.6 branch. So this will be fixed latest in the 1.7 release. Not sure if it can still get into 1.6.1, also I don't know why it wasn't put in there - probably because it was part of a larger patch which also added a few features.

[pc0de]

Alright, now you're just playing with me.

I'm also working against the trunk and it still has the "if(!p)" test. From my ".svn/entries":

Code: Select all

2943
https://irrlicht.svn.sourceforge.net/svnroot/irrlicht/trunk

Maybe I'm missing something?

[CuteAlien]

Oops - sorry. I had opened the svn 1.6 and the svn trunk version both in my editor, but seems I confused them :-)

So the correct info is: It's already fixed in 1.6 svn and will be in the 1.6.1 release. And it just hasn't been ported yet to trunk, which will be done before the 1.7 release. The (!p) check is still the same (the check was ok), but the last line has changed.

[hybrid]

Oh, I thought I already merged this stuff... Guess I need to double check this.

[pc0de]

My bad, I didn't look at the 1.6 source... Thanks to both of you for the attention.

[hybrid]

Ok, I haven't merged for some days. Should be done in the next hours

[pc0de]

Just a quick note to let you know that the following update in 1.6:

Code: Select all

Index: source/Irrlicht/CFileSystem.cpp
===================================================================
--- source/Irrlicht/CFileSystem.cpp     (revision 3123)
+++ source/Irrlicht/CFileSystem.cpp     (working copy)
@@ -480,7 +480,7 @@

 #endif

-       return io::path(filename);
+       return io::path(p);
 }

hasn't yet made it into the trunk or the 1.7 release branch. Thanks.

[CuteAlien]

Actually it seems it had made it into trunk already - and then was overwritten again by another change in revision 2913.
And yes - now it's broken again :-(

Thanks for reporting once more. Unfortunately I'm not sure right now why that part was changed again in 2913 - I hope hybrid still remembers.

[hybrid]

I was also pretty sure that it worked at some point. I'll check the things, but IIRC 1.7 did make some things different at other places, and didn't need the full changes anymore.
Well, looks like this "fix" was pretty broken, maybe it was a merge conflict which I did not resolve correctly or something. Anyway, new version should work under Windows and Linux now, maybe I will add the test next

[CuteAlien]

Just noting that the result is not the same as with the patch-proposal from pc0de.

For the code:

Code: Select all

#include <irrlicht.h>

using namespace irr;
using namespace io;

#ifdef _IRR_WINDOWS_
#pragma comment(lib, "Irrlicht.lib")
#endif

int main()
{
	video::E_DRIVER_TYPE driverType = video::EDT_NULL;
	IrrlichtDevice * device = createDevice(driverType, core::dimension2d<u32>(640, 480));
	if (device == 0)
	  return 1; // could not create selected driver.

	io::IFileSystem * fs = device->getFileSystem();

	path apath = fs->getAbsolutePath ("../nothere.txt");   // file does not exist
    printf("../nothere.txt: %s\n", apath.c_str());

    apath = fs->getAbsolutePath("../Demo");
    printf("../Demo: %s\n", apath.c_str());

    apath = fs->getAbsolutePath("../Demo/");
    printf("../Demo/: %s\n", apath.c_str()); 
      
	device->drop();

   return 0;
}

I get now that output on Linux:

Code: Select all

../nothere.txt: /home/micha/official_irrlicht/linux/irrlicht/branches/releases/1.7/examples/nothere.txt
../Demo: /home/micha/official_irrlicht/linux/irrlicht/branches/releases/1.7/examples/Demo
../Demo/: /home/micha/official_irrlicht/linux/irrlicht/branches/releases/1.7/examples/Demo

Which means not existing files get a path added. I have no idea if that is better/worse as I would probably prefer a 3rd alternative. Looking at the code I suspect it might return now an empty path on Windows (but I will have to check that when I boot Windows at some time unless someone does it before me). And that would imho make more sense than trying to give it our best guess as is done on Linux, because an empty result allows for error-checking which we are losing otherwise unlike the corresponding system functions.

I would change it once more, but maybe I miss something - so more feedback welcome...

[hybrid]

Well, if the test goes through for you it should work the same under Windows and Linux. Moreover, having a file "../something.txt" is pretty obvious to make absolute, knowing the current directory. There's no reason to check for existence of the files through this function - after all we have a separate existence check.

[CuteAlien]

Ok, I just checked documentation again and it seems that _fullpath can handle invalid files/paths and still return something, so you are right and it will also return some path on Windows in that case.