Bugfix ofToDataPath using Poco::Path #1523

Merged
merged 12 commits into from Jul 16, 2013

7 participants

@elliotwoods

Replaces #1251

Closes #1522, #1250, #819

Needs serious testing across all platforms.
Works fine here on osx first time (so i might have overlooked something)

Using Poco::Path we seriously clear up a bunch of folder functions.

I've also introduced ofSetWorkingDirectoryToDefault() and moved the call for it to ofRunApp, which should resolve #1522

Also I'm starting to think that ofDirectory should be returning Poco::Path derivatives not Poco::File derivatives.

I'm not sure here if windows system dialogs changing the cwd are fixed. Will look more now, but eyes on early!

@elliotwoods

ahh, there is 1 issue...
it always returns an absolute path right now since i'm storing the dataPath as absolute (storing it that way solves our cwd changing problems of course, but some still want it dearly)

Will check this out
EDIT: fixed

@ofTheo
@bilderbuchi
openFrameworks member

so, should I close #1251 if it is replaced by this one? Not that someone merges it when we actually want to integrate this one...

@elliotwoods

@ofTheo - the issue that i was having was ofFile actually loading the files into memory.
How often are we updating Poco btw?
anyway. back on topic

any chance you could test this on your multi-test setup?

@ofTheo

checking!
@julapy would be great if you could check on iPhone too! thanks.

@ofTheo

ps: I think this might be the scariest Pull Request to date!
:)

@ofTheo

@elliotwoods
Looking through this the logic is a little hard to follow. my instinct tells me there must be a bug or two in here, just from the number of changes and the number of things to take into account.

Some comments might be helpful so make it easier to review.

@damiannz - would be great to get your eagle eye on this diff.

@ofTheo

hmm - @elliotwoods
something with this PR completely messes up ofSort and ofRemove ( from ofDirectory usage ).

I'm getting crashes with both on CodeBlocks with this branch and crashes with ofSort on OS X #1526.
I thought it was unrelated - but switching back to develop the crashes go away.

can you try running the dirListExample?

on VS2010 I'm getting Poco::PathNotFoundException with dirListExample

@elliotwoods

Should #1526 be closed then if it's only related to this PR?
I'll look through VS and osx tomorrow (is that still in time, eeek!)
I thought we were banned from adding comments to the core? I think that makes sense in terms of usage, but comments are valuable to leave a trail of debugging breadcrumbs for core development

Any objections to adding comments to the core which don't otherwise belong in ofDocumentation?

@ofTheo

oh no - comments are welcome especially with something complex like this, where other people might need to understand the logic.

Lets keep that issue open, just incase its incidental somehow - we can close it when this PR is updated.

Cheers!
Theo

@elliotwoods

Ok this issue seems to be fixed now (dirListExample works fine)

I've fixed for the following cases in ofToDataPath:

  • input path already being inside data path
  • input path being formatted as a file even if it's a folder

Since this uses Poco::Path we should be able to remove all platform specific code from core, and it should be able to deal with any file locator on any platform (e.g. including network paths).

@ofTheo

okay - great, going to test it!

@bilderbuchi
openFrameworks member

Since this uses Poco::Path we should be able to remove all platform specific code from core, and it should be able to deal with any file locator on any platform (e.g. including network paths).

that sounds great, would potentially also significantly reduce the headache with paths in the project generator? @ofZach ?

@ofTheo

Okay so abs paths seems broken for me now on OS X. Before this last commit this path worked fine.

//--------------------------------------------------------------
void testApp::setup(){

    dir.listDir("/Users/theo/Desktop/images/of_logos/");

putting ofDisableDataPath(); at the top of the app allows for the abs path to work.

@elliotwoods

that was a pretty stupid error i introduced. fixed

@ofTheo

hey @elliotwoods - just checked this on windows.

everything seemed to work well except one thing.

if you do

ofSetDataPathRoot("C:/Users/theo/Desktop"); 

it doesn't currently add a trailing slash and you aren't able to load things from that directory.
with:

ofSetDataPathRoot("C:/Users/theo/Desktop/"); 

it works fine.

so maybe we need a check for trailing slash? and make sure we don't add a trailing slash for empty paths as that would then set the data path to root. ofFilePath::addTrailingSlash might handle this logic - be good to double check though.

going to test on os x

@underdoeg @bilderbuchi - would you be able to test linux?
just try the dirList example and try moving the images/of_logos/ folder around your HD and setting different dataPathRoots etc ( generally try and make it break :)

@bilderbuchi
openFrameworks member

I can test, but tomorrow at the earliest.

@underdoeg

Sorry, same for me, I can try it tomorrow...

@damiannz damiannz commented on the diff Sep 3, 2012
libs/openFrameworks/utils/ofUtils.cpp
- dataPathRoot() = newRoot;
- isDataPathSet() = true;
+//--------------------------------------------------
+void ofSetDataPathRoot(string newRoot){
+ dataPathRoot() = Poco::Path(newRoot);
@damiannz
damiannz added a note Sep 3, 2012

i realise this was here before but it's making me squint really hard.

can dataPathRoot() return a pointer rather than a reference, then we dereference it? still weird but not quite as weird.

@damiannz
damiannz added a note Sep 3, 2012

cf @arturoc 's comment above, how about:

string& rootReference = dataPathRoot();
rootReference = Poco::Path(newRoot);

much easier to read in any case.

Poco::Path & rootReference = dataPathRoot();
rootReference = Poco::Path(newRoot);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@damiannz damiannz commented on the diff Sep 3, 2012
libs/openFrameworks/utils/ofUtils.cpp
#endif
+#endif
@damiannz
damiannz added a note Sep 3, 2012

what about Windows, Linux?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@damiannz damiannz commented on the diff Sep 3, 2012
libs/openFrameworks/utils/ofUtils.cpp
- if (!isDataPathSet())
- ofSetDataPathRoot(dataPathRoot());
+ // if our Current Working Directory has changed (e.g. file open dialog)
+ if (defaultWorkingDirectory().toString() != getWorkingDir().toString()) {
@damiannz
damiannz added a note Sep 3, 2012

this doesn't look safe, or rather looks ripe for bugs in the future. does Poco::Path not have a built-in == operator, or an equivalence function? the string "/usr/local/bin" != the string "/usr/local/../local/../local/bin", but the paths represent the same thing.

@damiannz
damiannz added a note Sep 3, 2012

also, symlinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@damiannz damiannz commented on the diff Sep 3, 2012
libs/openFrameworks/utils/ofUtils.cpp
- #endif
- }
-
+ Poco::Path const & dataPath(dataPathRoot());
+ Poco::Path inputPath(path);
+ Poco::Path outputPath;
+
+ // if path is already absolute, just return it
+ if (inputPath.isAbsolute()) {
+ return path;
+ }
+
+ // here we check whether path already refers to the data folder by looking for common elements
+ // if the path begins with the full contents of dataPathRoot then the data path has already been added
+ // we compare inputPath.toString() rather that the input var path to ensure common formatting against dataPath.toString()
+ string strippedDataPath = dataPath.toString();
@damiannz
damiannz added a note Sep 3, 2012

the Poco docs are vague about toString(), specifically what happens when a Path's isRelative() returns true. vagueness means one can't make assumptions about it. again '/usr/local/bin' == '/usr/local/../local/bin'. and if i have a symlink, `/home/damian/linky' perhaps == '/usr/local/bin'.

to be completely robust we can't simply compare strings.

but, if we choose to ignore the existence of symlinks, i think this can work if all the paths in question have .resolve() called on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@damiannz

in general, i think this argues for basing ofFile on Poco::Path. for example a function path.isSubpathOfPath( otherPath ) would be handy and if robustly implemented once would solve a couple of problems.

collated comments:

. comparing strings in order to determine path equivalence isn't robust or safe. Poco docs don't specify the handling of relative paths, nor resolution (or awareness) of symlinks.

. relative path CWD is missing on Linux and Windows. with Linux i know we can get the path to the executable out of argv[0], i would expect the same to be true on Windows.

@arturoc
openFrameworks member

about CWD, arg[0] returns the command used to start the program which doesn't necessarily include the whole path to it. And we already have ofFilePath::getCurrentWorkingDirectory() which is multiplatform. There's also a couple of functions in ofFilePath: getCurrentExeDir() and getCurrentExePath() that return the path to the executable and it's directory. in linux i use those to find the data folder no matter where the executable is started from.

also comparison operators in poco doesn't seem to work, i've fixed the ones in ofFile and ofDirectory to work even if their paths are different but they point to the same place so instead of using Poco::Path you can use ofDirectory to do those checks

@underdoeg

I tried to compile your branch on ubuntu 64. But I got the following error:

 ../../../openFrameworks/utils/ofUtils.cpp:223:22: error: ‘MAXPATHLEN’ was not declared in this scope
 ../../../openFrameworks/utils/ofUtils.cpp:224:9: error: ‘charWorkingDir’ was not declared in this scope

Any ideas why? Sorry, I don't have the time right now to check out myself what is wrong...

@ofTheo

yup - you have to add ( under the includes )

#ifndef MAXPATHLEN
#define MAXPATHLEN 1024
#endif

@arturoc arturoc commented on the diff Sep 3, 2012
libs/openFrameworks/utils/ofUtils.cpp
@@ -194,105 +194,109 @@ void ofDisableDataPath(){
}
//--------------------------------------------------
-//use ofSetDataPathRoot() to override this
-static string & dataPathRoot(){
@arturoc
openFrameworks member
arturoc added a note Sep 3, 2012

this function should return a reference to an internal pointer of a variable in the heap if not it can cause problems with constructors, destructors or other static variables calling it: http://www.parashift.com/c++-faq/static-init-order-on-first-use-members.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@underdoeg

Worked ok for me on linux. I tried "images/of_logos", "/home/username/of_logos", "../../../../../../../of_logos/" with and without trailing "/"

@ofTheo
@underdoeg

" ofSetDataPathRoot("/home/username");
and then try and load "of_logos/"
"

That works as well

@elliotwoods

is the feature provocation sufficient for it to sink into oF from here?
i'm not setup here to do much testing / laptops are still too slow

let me know where I can help from now on.
if it still needs pushing through then i'm happy to do that, e.g. i can make any changes

but might be good to have someone else's hands on it to make sure i haven't introduced errors
it was a pretty big change (especially changing away from strings, which I'm very happy with as we shouldn't be dealing with strings at all, e.g. shouldn't be using toString() (i had an issue finding an equivalence operator.
The class Poco::Path is a bit incomplete (e.g. can't find a relative path given 2 absolute paths, defaulting to absolute if none available)))

@bilderbuchi
openFrameworks member

well, as far as I can tell all the things Damian and Arturo commented on above are still unchanged in the PR.

@elliotwoods

@bilderbuchi - I meant "is it better for me to stop coding here and someone else to complete before merging?"

@bilderbuchi
openFrameworks member

How do we proceed from here? do we try to get this into 0072, or push it back? @ofTheo ?

@arturoc
openFrameworks member

i've done some work on this, need to do some more testing and it'll need to be tested on other platforms so i think better to leave it for 0073

@elliotwoods

@arturoc - still looking at this? i can do some more testing here

i agree static string & dataPathRoot(){ is bad syntax, but that was there before me, so i left it in :).

let me know if i'm free to look into this, or you've got something cooking in the background instead

@bilderbuchi
openFrameworks member

In light of the upcoming bugfixing drive towards 0.8 this weekend, could we get an update on the status of this PR? Will we be able to finish/merge this soon?
Thanks!

@elliotwoods

hey all!

i'm using this branch on osx+vs (and installing this branch on anybody's computer who's using git + visual studio)
Currenlty this PR is necessary for me on windows and doesn't introduce any new bugs

the points which @arturoc and @damiannz made above are certainly valid, but are not issues which are introduced in this PR (string comparison of paths and using a function retuned static as a global setting var were both there before me).

The biggest issue is cross-platform checks.
Can somebody test on linux?
i can make this into a new branch ready to merge with develop also

Elliot

@bilderbuchi
openFrameworks member

there weren't any fresh commits to this PR since @underdoeg successfully tested on linux 10 months back, so I don't expect surprises. If it still works with current develop we can only judge when it's mergeable.

@bilderbuchi
openFrameworks member

0.8.0 is a couple of days away. @elliotwoods can we still get this in before?

@kylemcdonald

@elliotwoods this is your chance! if you really believe in your PR, you have to keep up the good fight! update the fix for the latest develop and we'll merge it! otherwise we're pushing it back to 0.8.1!

@elliotwoods

Hey!

This now works for me on windows and osx.

@arturoc arturoc commented on an outdated diff Jul 16, 2013
libs/openFrameworks/utils/ofUtils.cpp
- //printf("executable path is %s\n", path);
- string pathStr = string(path);
-
- //theo: check this with having '/' as a character in a folder name - OSX treats the '/' as a ':'
- //checked with spaces too!
-
- vector < string> pathBrokenUp = ofSplitString( pathStr, "/");
-
- newPath = "";
-
- for(int i = 0; i < pathBrokenUp.size()-1; i++){
- newPath += pathBrokenUp[i];
- newPath += "/";
- }
+static Poco::Path & dataPathRoot(){
+ static Poco::Path dataPathRoot(defaultDataPath());
@arturoc
openFrameworks member
arturoc added a note Jul 16, 2013

can you change singletons like this to:

    static Poco::Path * dataPathRoot = new Poco::Path(defaultDataPath()) ; 
    return *dataPathRoot;

otherwise this can crash on applications trying to get the dataPath when the application is closing, like some app trying to save settings before finishing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elliotwoods

i've incorporated @arturoc's comments re: singleton pointers vs references

@arturoc arturoc and 1 other commented on an outdated diff Jul 16, 2013
libs/openFrameworks/utils/ofUtils.cpp
}
-static bool & isDataPathSet(){
- static bool * dataPathSet = new bool(false);
- return * dataPathSet;
+//--------------------------------------------------
+static Poco::Path & defaultWorkingDirectory(){
+ static Poco::Path defaultWorkingDirectory;
+ return defaultWorkingDirectory;
@arturoc
openFrameworks member
arturoc added a note Jul 16, 2013

can you change this one too? i think it's the last one but in general any static variables that can be accessed while the app is finishing should be created in the heap and never destroyed

ok, this is odd
that change works fine in vs
but in xcode, it does work with -O0 but doesn't work with -O3
i.e. as a pointer it doesn't work in -O3 on xcode, but works on vs + xcode

i thought this could be a case of Poco playing strange? but i'm using the same debug libs in -O3 -O0 tests.

Is there an alternative way of approaching statics other than the pointer method?

@arturoc
openFrameworks member
arturoc added a note Jul 16, 2013

no, what do you mean that it doesn't work? do you get a crash or it doesn't return the correct thing...?

ok. clean+rebuild fixed it
(sorry about that)

it was exiting with 'image not found' which i presumed was an error from the example that i was testing with if the image (picture) path didn't exist, but it was a library error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kylemcdonald

awesome, so we're good now? all @arturoc's comments resolved? running fine on osx + windows? :)

@kylemcdonald kylemcdonald merged commit 97df2a2 into openframeworks:develop Jul 16, 2013
@elliotwoods elliotwoods deleted the unknown repository branch Jul 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment