Refactor src/license.jl #251
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #251 +/- ##
==========================================
+ Coverage 67.98% 68.08% +0.10%
==========================================
Files 6 6
Lines 3314 3290 -24
==========================================
- Hits 2253 2240 -13
+ Misses 1061 1050 -11 ☔ View full report in Codecov by Sentry. |
| if isdir(dirname(libxprs)) | ||
| cd(dirname(libxprs)) | ||
| end |
There was a problem hiding this comment.
we can try removing this. We´d need to test the PR before merging
There was a problem hiding this comment.
I think I've done it correctly, but try testing it.
| # Now you can use Xpress | ||
| ``` | ||
| """ | ||
| function initialize(; kwargs...) |
There was a problem hiding this comment.
@joaquimg any reason to call userlic instead of this initialize? We'll keep the old behavior, but this might be simpler. No need for dlopen and XPRSinit.
There was a problem hiding this comment.
I have had to load some libraries explicitly in some cases. For instance, xprl.dll on windows.
There was a problem hiding this comment.
Hmm. We should be able to fix that. The CI binaries work find? I think it should probably also work without the explicit dlopen. ccall should do it.
There was a problem hiding this comment.
I can simply start using this new reasonable function and report back
There was a problem hiding this comment.
So I removed the dlopen and CI still passing. So let me know
There was a problem hiding this comment.
it would be good to document how to swap versions with he jll.
It is very common that we need specific versions to reproduce and debug
There was a problem hiding this comment.
how does it find the dll with no dlopen?
libxprs is the full path, and that should be able to open any dependencies.
it would be good to document how to swap versions with he jll.
This PR is unrelated to the jll?
There was a problem hiding this comment.
This is working on my naive tests. Will test in production during the week!
There was a problem hiding this comment.
Since we're going to need to do a bunch of testing before tagging the next release (which I'll make as breaking, just to be on the safe side), objections to merging this now so you can test on master or with the Xpress_jll patch?
There was a problem hiding this comment.
Let's wait Monday night. Today is Sunday in Brazil :)
Take 2 of #240
I'll comment inline.