Skip to content

Actually fix File.symlink on Windows#6762

Merged
headius merged 3 commits intojruby:masterfrom
headius:really_fix_windows_symlink
Jul 26, 2021
Merged

Actually fix File.symlink on Windows#6762
headius merged 3 commits intojruby:masterfrom
headius:really_fix_windows_symlink

Conversation

@headius
Copy link
Member

@headius headius commented Jul 26, 2021

It seems like this code was broken before I messed with it. The
FFI::Config constant does not exist, so this was not actually
defining the FFI-based File.symlink. As a result, my change to
load that logic lazily caused a stack overflow.

The changes here fix this:

  • Always define the Windows-specific String function present here.
  • Only check for the FFI constant to know if FFI is available.
  • If FFI is not available or it cannot bind the necessary
    functions, instead define File.symlink to raise NotImplemented.

See #6751.

It seems like this code was broken before I messed with it. The
FFI::Config constant does not exist, so this was not actually
defining the FFI-based File.symlink. As a result, my change to
load that logic lazily caused a stack overflow.

The changes here fix this:

* Always define the Windows-specific String function present here.
* Only check for the FFI constant to know if FFI is available.
* If FFI is not available *or* it cannot bind the necessary
  functions, instead define File.symlink to raise NotImplemented.

See jruby#6751.
@headius headius added this to the JRuby 9.3.0.0 milestone Jul 26, 2021
headius added 2 commits July 26, 2021 15:16
In jruby#4349 we saw that this logic does not load properly when native
function calls have been disabled, due to FFI partially loading
and defining an FFI module that was not fully functional. This
is why the previous code here checked for the existence of
FFI::Config rather than just FFI.

However this is still error-prone, so I am restructuring this
logic to do all FFI juggling within the same rescue block as the
require of 'ffi'. If that require raises LoadError or if the
required functions are not present, it immediately falls back on
defining the NotImplemented version of File.symlink.

The same change has been done to the Solaris File#flock logic
below.
@headius headius requested a review from enebo July 26, 2021 20:32
@headius headius merged commit 995b393 into jruby:master Jul 26, 2021
@headius headius deleted the really_fix_windows_symlink branch July 26, 2021 21:18
@mohits mohits mentioned this pull request Sep 16, 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.

1 participant