libuv_readlink_function #10714

peter1000 · 2015-04-01T18:15:01Z

libuv_readlink_function: This replaces the PR #10687.

As requested by @tkelman and @Keno in #10571: adds wrapper function in C in jl_uv.c as well as the necessary documentations and tests.

This is pre-required for #10506

make test file

[user@evo test]$ make test file
    JULIA test/test
     * test                 in   0.61 seconds
    SUCCESS
    JULIA test/file
     * file                 in   9.63 seconds
    SUCCESS
[user@evo test]$ 

julia> versioninfo()
Julia Version 0.4.0-dev+4125
Commit e10405d* (2015-04-01 15:04 UTC)
Platform Info:
  System: Linux (x86_64-unknown-linux-gnu)
  CPU: Intel(R) Core(TM) i3-2310M CPU @ 2.10GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Sandybridge)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.3

tkelman · 2015-04-01T22:14:13Z

base/fs.jl

+    return linkval
+end
+@windowsxp_only readlink(p::AbstractString, np::AbstractString) =
+error("WindowsXP does not support soft symlinks")


Unless the underlying libuv function errors on XP, there's no reason for us to make readlink error there.

tkelman · 2015-04-02T06:55:11Z

Interesting failure on appveyor - the readlink from libuv on a directory link apparently ends in \\ on Windows, but not *nix?

To summarize the symlink situation on Windows, roughly speaking:

Windows XP can't create symlinks.
Vista-or-newer can create symlinks, but it requires a non-default permissions level.
Creating a link to a directory instead of a file can be done on Vista-or-newer using an NTFS junction, which is slightly different than a symlink (but don't ask me exactly how because I'm not quite sure) but can be created without needing unusual permissions.

peter1000 · 2015-04-03T07:58:59Z

Should we at least enable readlink for none windows and fix #10506 because at the moment the cp command does not work for directories.

tkelman · 2015-04-03T20:40:29Z

doc/stdlib/file.rst

+
+   .. note::
+
+      This function raises an error under operating systems that do not support


I don't think this note is true, unless uv_fs_readlink errors internally on XP.

tkelman · 2015-04-03T20:44:16Z

Can you look into uv_fs_readlink and see if it's any different between JuliaLang/libuv and upstream libuv/libuv?

We could modify the test to account for the OS-dependent behavior here, but would be good to track down where it's coming from.

And looking at this all-in-C version, I'd like someone else's opinion whether it's actually better in the end to do it this way (malloc'ing a string in C) versus the all-in-Julia way from #10571 (malloc'ing a uv_fs in Julia).

peter1000 · 2015-04-04T02:13:45Z

We could modify the test to account for the OS-dependent behavior here, but would be good to track down where it's coming from.

I only have access to Linux: so I can not be of much help on the other OS.

Can you look into uv_fs_readlink and see if it's any different between JuliaLang/libuv and upstream libuv/libuv?

I can check that tomorro or so..

peter1000 · 2015-04-04T15:57:28Z

hi @tkelman seems your suggestion worked. All is well — 2 successful checks

Anyway I had a look into the upstream libuv windows: fs.c A diff to the one used by julia gives a couple of changes.

I copy past the most relevant

@@ -287,7 +280,7 @@ INLINE static int fs__readlink_handle(HA
            (w_target[4] >= L'a' && w_target[4] <= L'z')) &&
           w_target[5] == L':' &&
           (w_target_len == 6 || w_target[6] == L'\\')) {
-        /* \??\«drive»:\ */
+        /* \??\<drive>:\ */
         w_target += 4;
         w_target_len -= 4;

@@ -296,8 +289,8 @@ INLINE static int fs__readlink_handle(HA
                  (w_target[5] == L'N' || w_target[5] == L'n') &&
                  (w_target[6] == L'C' || w_target[6] == L'c') &&
                  w_target[7] == L'\\') {
-        /* \??\UNC\«server»\«share»\ - make sure the final path looks like */
-        /* \\«server»\«share»\ */
+        /* \??\UNC\<server>\<share>\ - make sure the final path looks like */
+        /* \\<server>\<share>\ */
         w_target += 6;
         w_target[0] = L'\\';
         w_target_len -= 6;
@@ -312,8 +305,8 @@ INLINE static int fs__readlink_handle(HA
     w_target_len = reparse_data->MountPointReparseBuffer.SubstituteNameLength /
         sizeof(WCHAR);

-    /* Only treat junctions that look like \??\«drive»:\ as symlink. */
-    /* Junctions can also be used as mount points, like \??\Volume{«guid»}, */
+    /* Only treat junctions that look like \??\<drive>:\ as symlink. */
+    /* Junctions can also be used as mount points, like \??\Volume{<guid>}, */
     /* but that's confusing for programs since they wouldn't be able to */
     /* actually understand such a path when returned by uv_readlink(). */
     /* UNC paths are never valid for junctions so we don't care about them. */
@@ -359,7 +352,7 @@ INLINE static int fs__readlink_handle(HA
   /* If requested, allocate memory and convert to UTF8. */
   if (target_ptr != NULL) {
     int r;
-    target = (char*) malloc(target_len + 1);
+    target = (char*) uv__malloc(target_len + 1);
     if (target == NULL) {
       SetLastError(ERROR_OUTOFMEMORY);
       return -1;

@@ -1473,7 +1640,7 @@ static void fs__symlink(uv_fs_t* req) {
 static void fs__readlink(uv_fs_t* req) {
   HANDLE handle;

-  handle = CreateFileW(req->pathw,
+  handle = CreateFileW(req->file.pathw,
                        0,
                        0,
                        NULL,

tkelman · 2015-04-04T23:32:24Z

The private fields of uv_fs_t are slightly rearranged there between the JuliaLang fork and upstream, but otherwise doesn't look much different. It might be worth reporting this discrepancy upstream then?

I think this is acceptable, but again I'd like a second opinion (@Keno maybe?) whether we should prefer the C approach here or the Julia version in #10571. This PR has improved tests and docs, but we may want to go back to the other PR's implementation of readlink. Sorry for my indecisiveness.

peter1000 · 2015-04-05T08:37:13Z

Sorry for my indecisiveness.

No problem. I tried running a test on both version - it seems they allocate the same amount as well the execution time was quite the same.

If you decide on the other PR I would recommend calling the uv_error("readlink", ret) part a bit earlier to get a prober error message and avoid the problem I mentioned here. ERROR: LoadError: ArgumentError: cannot convert NULL to string

Cheers
P

peter1000 · 2015-04-07T12:03:16Z

hi @Keno any thoughts how to preceed?

tkelman · 2015-04-07T18:12:06Z

Maybe @vtjnash would respond faster. C version here, or Julia version from #10571 (but tweaked to move the uv_error a few lines earlier)?

tkelman · 2015-04-14T08:15:09Z

It looks like this needs a rebase.

Can someone who knows the libuv interface please respond with an opinion?

If no one says anything, I think I'm leaning towards the Julia version, and would just merge that absent an opinion from anyone else.

peter1000 · 2015-04-14T08:47:22Z

I think the julia version would be fine do. I could after it is actually merged the docs if you want me.

tkelman · 2015-04-16T14:24:59Z

Do you know how to do an interactive rebase and force push to this branch with git? I could manually merge the two changes but I've got a bunch of other work to do, it would be simplest if you can resolve the conflicts.

peter1000 · 2015-04-16T22:32:11Z

@tkelman sorry this was the first time I tried to rebase a PR. I had hoped it will be one final commit and not 4. Not sure what I did wrong.

tkelman · 2015-04-17T00:18:05Z

One step at a time, so far it looks like you were able to rebase to master correctly to resolve the merge conflict, so that's good. To combine multiple commits into one you select "squash" during the interactive rebase. And I think my vote will be to the Julia implementation of readlink, since at least that catches return codes and does all the allocation and freeing in one place instead of part in C and part in Julia to get a string across the boundary.

peter1000 · 2015-04-17T00:21:43Z

sorry I misunderstood that - will give it a try

Adds wrapper function in C in jl_uv.c as well as the necessary documentations and tests.

tkelman · 2015-04-17T02:15:32Z

This looks perfect, if it passes CI and nobody says otherwise I'll merge it. Sorry this took so long, would've been good to get input from another reviewer here.

ihnorton · 2015-04-17T02:45:07Z

I took a quick look through the uv code and readlink man page, and this looks correct to me (for whatever it's worth).

libuv_readlink_function

peter1000 · 2015-04-18T15:08:21Z

hi @tkelman
I just thought about: do we need to add anything for funtion readlink to the NEWS.md?
If so, what and where?

tkelman · 2015-04-18T15:13:08Z

Might be worth adding if you want, I'd say under Library improvements, other improvements

tkelman reviewed Apr 1, 2015
View reviewed changes

ghost mentioned this pull request Apr 2, 2015

WIP: readlink function #10571

Closed

tkelman reviewed Apr 3, 2015
View reviewed changes

libuv_readlink_function

b3c9ce1

Adds wrapper function in C in jl_uv.c as well as the necessary documentations and tests.

tkelman added a commit that referenced this pull request Apr 17, 2015

Merge pull request #10714 from peter1000/libuv_readlink_function

58cef56

libuv_readlink_function

tkelman merged commit 58cef56 into JuliaLang:master Apr 17, 2015

peter1000 deleted the libuv_readlink_function branch April 17, 2015 12:49

ihnorton mentioned this pull request Apr 17, 2015

Windows nightly 64-bit release exe does not start #10865

Closed

peter1000 mentioned this pull request Apr 18, 2015

Adds addition of readlink under Other improvements #10882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libuv_readlink_function #10714

libuv_readlink_function #10714

peter1000 commented Apr 1, 2015

tkelman Apr 1, 2015

tkelman commented Apr 2, 2015

peter1000 commented Apr 3, 2015

tkelman Apr 3, 2015

tkelman commented Apr 3, 2015

peter1000 commented Apr 4, 2015

peter1000 commented Apr 4, 2015

tkelman commented Apr 4, 2015

peter1000 commented Apr 5, 2015

peter1000 commented Apr 7, 2015

tkelman commented Apr 7, 2015

tkelman commented Apr 14, 2015

peter1000 commented Apr 14, 2015

tkelman commented Apr 16, 2015

peter1000 commented Apr 16, 2015

tkelman commented Apr 17, 2015

peter1000 commented Apr 17, 2015

tkelman commented Apr 17, 2015

ihnorton commented Apr 17, 2015

peter1000 commented Apr 18, 2015

tkelman commented Apr 18, 2015


		.. note::

		This function raises an error under operating systems that do not support

libuv_readlink_function #10714

libuv_readlink_function #10714

Conversation

peter1000 commented Apr 1, 2015

tkelman Apr 1, 2015

Choose a reason for hiding this comment

tkelman commented Apr 2, 2015

peter1000 commented Apr 3, 2015

tkelman Apr 3, 2015

Choose a reason for hiding this comment

tkelman commented Apr 3, 2015

peter1000 commented Apr 4, 2015

peter1000 commented Apr 4, 2015

tkelman commented Apr 4, 2015

peter1000 commented Apr 5, 2015

peter1000 commented Apr 7, 2015

tkelman commented Apr 7, 2015

tkelman commented Apr 14, 2015

peter1000 commented Apr 14, 2015

tkelman commented Apr 16, 2015

peter1000 commented Apr 16, 2015

tkelman commented Apr 17, 2015

peter1000 commented Apr 17, 2015

tkelman commented Apr 17, 2015

ihnorton commented Apr 17, 2015

peter1000 commented Apr 18, 2015

tkelman commented Apr 18, 2015