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.

Sign up for GitHub

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

Jump to bottom

Fix Issue #25137 #25175

Merged
merged 2 commits into from
Dec 20, 2017
Merged

Fix Issue #25137 #25175

merged 2 commits into from
Dec 20, 2017

Conversation

jdjohnston
Copy link
Contributor

  • Invert boolean value returned by Windows FreeLibrary so it matches
    that of POSIX dlclose
  • Add test of jl_dlclose/Libdl.dlclose to test/libdl.jl

@jdjohnston
Fix Issue JuliaLang#25137
e79f80c 
- Invert boolean value returned by Windows FreeLibrary so it matches
  that of POSIX dlclose
- Add test of jl_dlclose/Libdl.dlclose to test/libdl.jl
@jdjohnston jdjohnston mentioned this pull request Dec 19, 2017
Closed
@staticfloat
Copy link
Member

This looks great to me, thanks for your contribution @jdjohnston!

@kshyatt kshyatt added the system:windows Affects only Windows label Dec 19, 2017
@StefanKarpinski
Copy link
Member

libdl test failure on Win32 looks related; Win64 passed.

staticfloat
staticfloat reviewed Dec 19, 2017
View reviewed changes
test/libdl.jl Outdated
@test dl != C_NULL

@test Libdl.dlclose(dl)
@test !Libdl.dlclose(dl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is failing on win32, according to Appveyor

@jdjohnston
Copy link
Contributor Author

If it's line #197 that's failing, perhaps Win32 FreeLibrary doesn't error on calling it twice on the same handle. That only makes sense to me if the refcnt was > 1 before the first call. Without a Win32 Windows machine, this is going to be difficult to debug.

If indeed it is line #197 and unless someone can suggest a more reliable method to have FreeLibrary fail (other than a NULL handle, which is trapped by jl_dlclose), I would recommend deleting the line from the PR. :(

@StefanKarpinski
Copy link
Member

I would recommend deleting the line from the PR.

Or just change it to @test_skip so that there's a reminder to fix it if possible at some point in the future. Not high urgency, but it would be nice at some point. It's also a little unclear if we really need this return value or not.

@jdjohnston
Copy link
Contributor Author

@staticfloat @StefanKarpinski , PR updated as recommended

@JeffBezanson JeffBezanson merged commit 617afff into JuliaLang:master Dec 20, 2017
@jdjohnston jdjohnston deleted the fix-25137 branch December 20, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticfloat staticfloat staticfloat approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
system:windows Affects only Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jdjohnston @staticfloat @StefanKarpinski @JeffBezanson @kshyatt