Assembly4: XLink absolute filepath problem

Discussion about the development of the Assembly workbench.
User avatar
Zolko
Posts: 1103
Joined: Mon Dec 17, 2018 10:02 am

Re: Assembly4: XLink absolute filepath problem

Postby Zolko » Mon Aug 31, 2020 7:52 pm

wmayer wrote:
Mon Aug 31, 2020 12:01 pm
When loading a project then inside App::DocInfo::get() an entry to a map is added with information about the document. As key the absolute file path with the soft link is used.

When trying to search for the further assembly files while loading the project the function App::DocInfo::restoreDocument() is used. In this function the path was created with QFileInfo::canonicalFilePath() and thus the soft link has been resolved to the real path. This path is used to search in the map but because it has no matching key the second file won't be loaded.
well, this is too technical for me, but do I understand correctly that different methods are used to access files, when saving, opening, linking, whatever ... and thus the behaviour is inconsistent: works most of the time, but not always ?

If this is the case, would it be possible to not do any smart magic for v0.19, propose only simple and reliable methods, whatever they are (you will know better than me), and leave room for improvement for v0.20 ? This will not allow all fancy use-cases, but at least will be rock solid and easy to explain/document.
try the Assembly4 workbench for FreCAD v0.19
install with Tools > Addon Manager > Assembly4 — tutorials here and here
wmayer
Site Admin
Posts: 16649
Joined: Thu Feb 19, 2009 10:32 am

Re: Assembly4: XLink absolute filepath problem

Postby wmayer » Mon Aug 31, 2020 8:46 pm

Zolko wrote:
Mon Aug 31, 2020 7:52 pm
well, this is too technical for me, but do I understand correctly that different methods are used to access files, when saving, opening, linking, whatever ... and thus the behaviour is inconsistent: works most of the time, but not always ?
It was only a problem in this specific case (PropertyXLink) because in one function QFileInfo::absoluteFilePath() and in another function QFileInfo::canonicalFilePath() was used. In the standard case this works as expected since for both QFileInfo classes the same result is returned. So, it only failed when working with symlinks -- insofar just on oversight of this use case.

And as said by realthunder the string is only used for comparison. The files are neither accessed nor opened this way. This happens inside the Document class which won't cause any inconsistent behaviour.
If this is the case, would it be possible to not do any smart magic for v0.19, propose only simple and reliable methods, whatever they are (you will know better than me), and leave room for improvement for v0.20 ? This will not allow all fancy use-cases, but at least will be rock solid and easy to explain/document.
The whole document saving & loading mechanism is rock solid. I am not aware of any serious problem in the last years.
User avatar
Zolko
Posts: 1103
Joined: Mon Dec 17, 2018 10:02 am

Re: Assembly4: XLink absolute filepath problem

Postby Zolko » Mon Aug 31, 2020 9:17 pm

wmayer wrote:
Mon Aug 31, 2020 8:46 pm
It was only a problem in this specific case (PropertyXLink) ... it only failed when working with symlinks
...
The whole document saving & loading mechanism is rock solid. I am not aware of any serious problem in the last years.
yes, I understood that. What I meant was the rock-solidness of App::Link references. And in this case, symlinks is a very common use-case (heck, I use them all the time)

So my request was to make the App::Link-ing rock-solid even with symlinks. And in this case, allowing /../ is a sure way of breaking it : it is enough to have a directory with all your CAD data (CAD/Project1, CAD/Project2 ...) and have there a library of parts (CAD/PartsLibrary). Then when accessing the library from asm_Project1, the link will be referenced by ./../PartsLibrary/part_12345.fcstd.

Now, if you put a short-cut of the assembly on your Desktop, going ./../ will not lead to the CAD/PartsLibrary directory and you'll loose all links to the PartsLibrary.

Which means that it is not only trivial to fool the system, but it's quite automatic to fool it even if you don't want to.

A possible workaround would be to use relative paths for saving if the linked document is under the current assembly document's directory, and use absolute paths if not. Or, may-be better, save both the absolute and the relative paths, and when restoring the link try first the relative path, and if that fails try the absolute path.
try the Assembly4 workbench for FreCAD v0.19
install with Tools > Addon Manager > Assembly4 — tutorials here and here
aapo
Posts: 213
Joined: Mon Oct 29, 2018 6:41 pm

Re: Assembly4: XLink absolute filepath problem

Postby aapo » Mon Aug 31, 2020 9:28 pm

wmayer wrote:
Mon Aug 31, 2020 8:46 pm
And as said by realthunder the string is only used for comparison. The files are neither accessed nor opened this way. This happens inside the Document class which won't cause any inconsistent behaviour.
Also, It seems to me that realthunder's amended PR should be the right fix for the bug, if your above analysis is correct. I mean, the return QFileInfo(myPos->first).absoluteFilePath(); should now make the path resolving between App::DocInfo::get() and App::DocInfo::restoreDocument() match now, which I feel should be the correct fix for the problem, as it makes the relative patch the correct shortest possible path, or did I understand it wrong? Or maybe that was your point all along, it's a bit difficult to follow! :D

Anyway, if that's the case I think the "comments" in the patch should be corrected to reflect out why absoluteFilePath() must be chosen (the reason being, that absoluteFilePath is used elsewhere for the comparison; or rather correctly expressed: for the path construction). I mean, in some other cases it might make sense to use canonicalFilePath(), but then it should be used in the other place, too. And maybe the comment should somehow note that, because it's not obvious that the chosen logic should match some other place in the code. I guess the current version of the PR is still preliminary in that regard?

Code: Select all

        else {
            // return QFileInfo(myPos->first).canonicalFilePath();
            return QFileInfo(myPos->first).absoluteFilePath();
        }
wmayer
Site Admin
Posts: 16649
Joined: Thu Feb 19, 2009 10:32 am

Re: Assembly4: XLink absolute filepath problem

Postby wmayer » Mon Aug 31, 2020 10:02 pm

Zolko wrote: yes, I understood that. What I meant was the rock-solidness of App::Link references. And in this case, symlinks is a very common use-case (heck, I use them all the time)
OK. Since the link reference is relatively new (it's now around for a year in master) the future has to show how robust it really is.
aapo wrote: Also, It seems to me that realthunder's amended PR should be the right fix for the bug, if your above analysis is correct. ...
Yes, I know that it fixes the issue and you may have realized that it's already merged. But before applying the PR I first wanted to confirm the described behaviour and analyzed what happened behind the scene.
Anyway, if that's the case I think the "comments" in the patch should be corrected to reflect out why absoluteFilePath() must be chosen (the reason being, that absoluteFilePath is used elsewhere for the comparison; or rather correctly expressed: for the path construction). I mean, in some other cases it might make sense to use canonicalFilePath(), but then it should be used in the other place, too
I haven't tested this explicitly but I think it's only a matter of using the methods consistently -- so instead of using consistently absoluteFilePath() it should work to consistently use canonicalFilePath().

I think a cleaner fix would be to implement an explicit function to create the path for the map. And each time when searching for an element this function must be invoked. This way it's guaranteed that the path is consistent because it doesn't depend on implementation details of other functions.
And maybe the comment should somehow note that, because it's not obvious that the chosen logic should match some other place in the code. I guess the current version of the PR is still preliminary in that regard?
It doesn't affect the logic of other places in the code because it's only used in the very local context of the PropertyXLink class.
User avatar
Zolko
Posts: 1103
Joined: Mon Dec 17, 2018 10:02 am

Re: Assembly4: XLink absolute filepath problem

Postby Zolko » Tue Sep 01, 2020 12:50 pm

wmayer wrote:
Mon Aug 31, 2020 10:02 pm
I think a cleaner fix would be to implement an explicit function to create the path for the map. And each time when searching for an element this function must be invoked. This way it's guaranteed that the path is consistent because it doesn't depend on implementation details of other functions.
That sounds very convincing: would that allow to select to use absolute paths or relative paths also ? If the look-up involves this function, then the save should also invoke it, and then it should be possible to select which method should be used for the link references, both in the save, open and map functions. Would that work ?

And then, later, when this abstraction layer is stable, "we" can add other path methods, like library search paths.
try the Assembly4 workbench for FreCAD v0.19
install with Tools > Addon Manager > Assembly4 — tutorials here and here
aapo
Posts: 213
Joined: Mon Oct 29, 2018 6:41 pm

Re: Assembly4: XLink absolute filepath problem

Postby aapo » Tue Sep 01, 2020 3:34 pm

wmayer wrote:
Mon Aug 31, 2020 10:02 pm
Yes, I know that it fixes the issue and you may have realized that it's already merged. But before applying the PR I first wanted to confirm the described behaviour and analyzed what happened behind the scene.
Great, noticed only now, but thanks! :D This should work perfectly at least for my use cases, but hopefully the LIBRARY_PATH expansions requested by Zolko will be added soon, also.

I think a cleaner fix would be to implement an explicit function to create the path for the map. And each time when searching for an element this function must be invoked. This way it's guaranteed that the path is consistent because it doesn't depend on implementation details of other functions.
Yes, that sounds like a very good idea. That'd also make the need for explaining code comments largely unnecessary, as there wouldn't be any tripping points left for future coders to step upon about the path issue.
wmayer
Site Admin
Posts: 16649
Joined: Thu Feb 19, 2009 10:32 am

Re: Assembly4: XLink absolute filepath problem

Postby wmayer » Thu Sep 03, 2020 10:23 am

Zolko wrote:
Tue Sep 01, 2020 12:50 pm
That sounds very convincing: would that allow to select to use absolute paths or relative paths also ?
In this topic two topics are discussed: the error reported by project and the handling of libraries. I was only talking about the error and my suggestion was to make realthunder's fix a bit more robust as it reduces the dependency of several functions.
If the look-up involves this function, then the save should also invoke it, and then it should be possible to select which method should be used for the link references, both in the save, open and map functions. Would that work ?
Maybe the handling of libraries should be discussed in a separate topic. You gave some ideas how to implement and an example of a commercial system where it works sufficiently well.

However, in this area I don't have much experience and can't really say what would be best.
User avatar
Zolko
Posts: 1103
Joined: Mon Dec 17, 2018 10:02 am

Re: Assembly4: XLink absolute filepath problem

Postby Zolko » Thu Sep 03, 2020 11:53 am

wmayer wrote:
Thu Sep 03, 2020 10:23 am
However, in this area I don't have much experience and can't really say what would be best.
For the specification and implementation of a library path, I think that will take some time to do it well. Therefore, I'd not worry about it right now.

What we should worry about on the other hand, is the absolute/relative path problem as discussed in this thread, and this should be addressed for the final v0.19 release. I very much would like to be able to set that in the preferences. There are good and legitimate use-cases for both, and both have been done in the past by FreeCAD, therefore the amount of work should be limited.

wmayer wrote:
Mon Aug 31, 2020 10:02 pm
I think a cleaner fix would be to implement an explicit function to create the path for the map.
I really would like your proposal be implemented. I don't know how it could be coded, but my wish would be that the user can choose between different methods for this map. For v0.19 the choice should be limited to absolute or relative paths, but at least the abstraction of this function should be there.

realthunder wrote:
Thu Aug 27, 2020 11:47 pm
ping
EDIT: forgot: what do you think ?
try the Assembly4 workbench for FreCAD v0.19
install with Tools > Addon Manager > Assembly4 — tutorials here and here
realthunder
Posts: 1810
Joined: Tue Jan 03, 2017 10:55 am

Re: Assembly4: XLink absolute filepath problem

Postby realthunder » Fri Sep 04, 2020 11:52 pm

Zolko wrote:
Thu Sep 03, 2020 11:53 am
What we should worry about on the other hand, is the absolute/relative path problem as discussed in this thread, and this should be addressed for the final v0.19 release. I very much would like to be able to set that in the preferences. There are good and legitimate use-cases for both, and both have been done in the past by FreeCAD, therefore the amount of work should be limited.
The main concern I have with absolute path is how to deal with the situation when the path no longer exist, like when the user moves the file to another computer where there is no d: ? Even on unix like system, the user may not have root access so he cannot use symlink as a work around. And of course it will be worse when moving files between windows and linux.

PropertyXLink can actually save absolute linked path. It is used when copying object from a different document. I originally wanted to expose this absolute/relative option to end user, but thought it is not practical to do it on per link basis, or at global level because of the concerns above. The library path solution can let user easily recreate the same directory layout on different systems. I think we should stick with that.
Try Assembly3 (latest version 0.11) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal