Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix symbolic file(or contains symbolic directory) check condition #519

Merged
merged 6 commits into from
Oct 20, 2021

Conversation

devjeonghwan
Copy link
Member

file.getCanonicalFile() is not solve symbolic link in Windows environment (See issue)

So i change to more precise method toRealPath().

@saudet
Copy link
Member

saudet commented Oct 15, 2021

Let's replace all the calls, not just that one. However, Path.toRealPath() doesn't appear available before Android 8.0:
https://developer.android.com/reference/java/nio/file/Path?hl=ro#toRealPath(java.nio.file.LinkOption...)
So let's create our own Loader.getCanonicalFile() that calls Path.toRealPath() only on Windows, and File.getCanonicalFile() on other platforms.

@devjeonghwan
Copy link
Member Author

I tested on Android 25 API Level.

@devjeonghwan
Copy link
Member Author

devjeonghwan commented Oct 15, 2021

Frequently compare platform string to "windows" seems slow... How about implementing a one-time check(consider volatile or syncronoized) or enum for platform?

@saudet
Copy link
Member

saudet commented Oct 16, 2021

We can use something like private static final boolean WINDOWS = PLATFORM.startsWith("windows"); under this line:
https://github.com/bytedeco/javacpp/blob/master/src/main/java/org/bytedeco/javacpp/Loader.java#L82

BTW, there are other places in the code where File.getCanonicalPath() gets called:
https://github.com/bytedeco/javacpp/search?q=getCanonicalPath

@devjeonghwan
Copy link
Member Author

devjeonghwan commented Oct 16, 2021

We can use something like private static final boolean WINDOWS = PLATFORM.startsWith("windows"); under this line: https://github.com/bytedeco/javacpp/blob/master/src/main/java/org/bytedeco/javacpp/Loader.java#L82

BTW, there are other places in the code where File.getCanonicalPath() gets called: https://github.com/bytedeco/javacpp/search?q=getCanonicalPath

@saudet How about like this?

    /** Value created out of "java.vm.name", "os.name", and "os.arch" system properties.
     *  Returned by {@link #getPlatform()} and initialized with {@link Detector#getPlatform()}. */
    private static final String PLATFORM = Detector.getPlatform();

    /**
     * Returned an enumeration of known operating systems and initialized with {@link Detector#getOperatingSystem()}.
     */
    private static final Detector.OperatingSystem OPERATING_SYSTEM = Detector.getOperatingSystem();

    public static class Detector {
        public enum OperatingSystem {
            WINDOWS, LINUX, ANDROID, MACOSX, IOS, OTHER;

            /**
             * @return Parsed pre-defined operating system enum
             */
            static OperatingSystem parse(String string) {
                if (string != null){
                    string = string.toLowerCase();

                    if (string.startsWith("windows")) {
                        return OperatingSystem.WINDOWS;
                    } else if (string.startsWith("linux")) {
                        return OperatingSystem.LINUX;
                    } else if (string.startsWith("android")) {
                        return OperatingSystem.ANDROID;
                    } else if (string.startsWith("macosx")) {
                        return OperatingSystem.MACOSX;
                    } else if (string.startsWith("ios")) {
                        return OperatingSystem.IOS;
                    }
                }

                return OperatingSystem.OTHER;
            }
        }

        public static OperatingSystem getOperatingSystem(){
            return OperatingSystem.parse(Detector.getPlatform());
        }
        ...
    }

I think it's better than making a field just for windows.

@saudet
Copy link
Member

saudet commented Oct 16, 2021

No, let's just leave it as a string. That just adds unnecessary complexity.

@devjeonghwan devjeonghwan requested a review from saudet October 16, 2021 06:07
@saudet saudet merged commit 28f89ad into bytedeco:master Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants