fix temporary directory handling on macOS #709
Conversation
Similarly to Windows, redirect `/tmp/…` paths accesses to the correct folder on macOS, fixing running the testsuite on the later.
|
See previous pull request #384 and related discussion. Which parts of the test suite fail in your tests? |
|
According to my tests, there is a regression which was introduced in @rsesek's pull request #583 by commit 05ff50e. That commit changed Leptonica must either stick to using The current mix of both alternatives causes the failing tests. |
|
There are more risks when using So that alternative uses a temporary directory which can only be accessed by its owner, and any file there can be cleaned by the system. So I see three reasons why Leptonica should use |
Running #708 without the fix or parallel testing, here are the failures with the Note: I don't have access to a macOS machine. |
Agreed. |
Except for that last part, all good things I would say. For that last part: are the files removed when the system is running? Also note the wording: "may be", so that might be conditional on other conditions (e.g. low disk space). |
Sandboxed macOS applications, which all modern Mac apps should be and which is also a requirement for submission into the Mac App Store, cannot access This is corroborated by Table 1-1 in the Mac App Programming Guide. That suggests using I do not think Leptonica should revert to using
The same "risks" apply to
I personally don't think this is a good reason for a library to not do the functionally correct thing. |
|
Thank you for taking the time and explaining your arguments.
So auto-clean is identical for both alternatives, and you don't think that the risk of breaking existing code because of the longer path (48 characters instead of 4) is a good reason why Leptonica should stick to |
|
By the way, I noticed that even Apple and Google still use /tmp, at least for xquartz and keystone. |
The difference in accessibility is the purpose of the secure, per-user temp directory and is the desired property. Why should these temp files be made available to all users on the system? System-wide temp directories are generally not desirable from security or privacy perspectives, which is why macOS introduced this feature.
Keystone is a legacy codebase, and the product that is replacing it, the Chromium Updater, uses the modern temp dir location. And most Apple software does too, because it obtains the temp dir path from the Foundation framework, on which most Mac software is based. |
|
Looking at the failures in the reg tests (only 2 out of 147), which were in compare_reg and rectangle_reg: they all involved the function lept_cp() or lept_mv(), that was doing a copy or a move from a named file in /tmp/lept/... And those are the only such calls in the regression test suite. I can't tell for sure, but my guess is that the files are not being written into the specified /tmp/lept/... path. Instead, the path is rewritten and the lept_cp() and lept_mv() fail because they are using the hardcoded paths. |
Dan, both |
|
It would be nice to resolve this. Here are my thoughts: (1) I like putting all the rewriting in Consequently my inclination is to merge 709 if it's not too risky in terms of running up against small buffers. If we do that, I'll make a stab at simplifying the |
I think that this pull request contains new code which is not needed. The initial version of #384 already covered iOS and macOS and was modified to handle iOS only because I thought that the risk of getting longer path names for temporary files should be avoided. If we now accept that risk, it is sufficient to enable the iOS code for macOS, too. |
|
OK, I admit this is far above my pay grade. I see your comment that changing line.3195 to only use iOS will fix the two tests for macOS, and is it true that we shouldn't care about testing on iOS? As you can see from my comments above, although I do not understand the level of risk for apps that use small buffers (for example, would any supported version of tesseract fail?), I do like the simplicity and consistency of the code that results from this PR. |
|
I understand your concern. Here is a compromise. We keep the functionality as is for macOS (using "/tmp" without rewrite) and have the Darwin rewrite only on iOS, as you suggest. We also adopt the reorganization in this PR that puts all rewriting directly into genPathname() and simplifies makeTempDirname(), lept_cp() and lept_mv(). I've attached my proposed utils2.c: |
|
I am closing this pull request because it has been superseded by PR #713. |
Similarly to Windows, redirect
/tmp/…paths accesses to the correct folder on macOS, fixing running the testsuite on the later.