Skip to content

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

Merged
saudet merged 6 commits into
bytedeco:masterfrom
devjeonghwan:master
Oct 20, 2021
Merged

Fix symbolic file(or contains symbolic directory) check condition#519
saudet merged 6 commits into
bytedeco:masterfrom
devjeonghwan:master

Conversation

@devjeonghwan
Copy link
Copy Markdown
Member

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

So i change to more precise method toRealPath().

@saudet
Copy link
Copy Markdown
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
Copy Markdown
Member Author

I tested on Android 25 API Level.

@devjeonghwan
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/main/java/org/bytedeco/javacpp/Loader.java Outdated
@devjeonghwan
Copy link
Copy Markdown
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
Copy Markdown
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