search

andreas-kupries / critcl

Critcl lets you easily embed C code in Tcl. Online documentation at
http://andreas-kupries.github.io/critcl
Other
72 stars 19 forks source link

Fix problems with previous commit, handle intent to install to isolated directory. #91

Closed pooryorick closed 5 years ago

pooryorick commented 5 years ago

The previous attempt to fix quoting issues in the generated script was bad. This pull request includes an approach that works.

Also, if "destination" is provided, e.g. /path/to/mycritcl/lib, and its parent doesn't exist, e.g. /path/to/mycritcl, install critcl entirely into /path/to/mycritcl, e.g. /path/to/mycritcl/bin, /path/tomycritcl/lib, etc.

andreas-kupries commented 5 years ago

I saw the 1st (now closed) PR yesterday evening

While I believe that I puzzled out the need for the path change (list quoting handles things like \, and the simple {...} I used will not cut in that case, and assuming your are on windows you can/will have paths with such), I have not managed yet to deduce what failure case prompted the complexification of the exe handling.

I did see it breaking execution on unix. ... Glancing at the this PR I see that you fixed that breakage. I am still puzzled by the need for it. Would appreciate it if you could provide examples of how the previous handling of exe goes wrong in your eyes, and how the change fixes that.

As for the new work in the 2nd PR, about installation with partially existing destination path, well, I would like to see examples of the different/changed behaviour as well. Given that I would likely changed the code to simply create the missing pieces of the destination path instead, instead of changing the semantics.

pooryorick commented 5 years ago

Regarding the app script and quoting, it was not a particular problem I experienced but the fact that a double quote in the executable name would have defeated the script, as it was. The update is robust against any possible quoting issues.

Regarding the installation path, "destination" was conceived as the library directory for critcl, so it works a little differently from the standard ./configure --prefix ..

Prior to the change, when a destination of /path/to/mycritcl/lib was provided, and there was existing /path/to/mycritcl/bin, critcl decided to install critcl to /bin while the intention was to install to /path/to/mycritcl/bin.

The change is that the installer creates /path/to/mycritcl/bin directory and friends if /path/to/mycritcl (the parent of destination) doesn't already exist. The condition of parent of destination not existing, is sufficient to differentiate this case from the more traditional cases.

pooryorick commented 5 years ago

This patch still has issues. Closing this request. A new one will follow.