-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
DNMY: Refactor and tidy src/license.jl #240
Conversation
Lib.XPRSlicense(lic, path_lic) | ||
buffer = Vector{Cchar}(undef, 1024 * 8) | ||
ierr = GC.@preserve buffer begin | ||
Lib.XPRSlicense(lic, Cstring(pointer(buffer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joaquimg any points on why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this buffer might not be needed, it probably came from a copy-paste from the usage below.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #240 +/- ##
==========================================
- Coverage 66.54% 66.48% -0.06%
==========================================
Files 6 5 -1
Lines 3405 3390 -15
==========================================
- Hits 2266 2254 -12
+ Misses 1139 1136 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not merge this as is. It is not clear what this is improving.
@@ -28,20 +28,64 @@ const libxprs = joinpath( | |||
include("Lib/Lib.jl") | |||
include("helper.jl") | |||
include("api.jl") | |||
include("license.jl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the idea of moving all this code to the root file.
A user/developer should not start their trip into express from this license code.
|
||
function initialize() | ||
Libdl.dlopen(libxprs) | ||
userlic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all our code relies on calling this. I would not like to deprecate unless extremely necessary.
buffer_p = pointer(buffer) | ||
ierr = GC.@preserve buffer begin | ||
Lib.XPRSlicense(lic, Cstring(buffer_p)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is necessary.
part of the authentication scheme relies on this for some license type.
first we call XPRSlicense to get an initial value for lic
, then we modify it in libcheck
(each user has its own password type function, some licenses don't need it, hence the empty default, but other do need it, and we use it at PSR).
then XPRSlicense is called again to verify that lic
was modified properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Let me take another go at this. It was just hard to tell why `libcheck was necessary because it wasn't documented or tested.
Lib.XPRSlicense(lic, path_lic) | ||
buffer = Vector{Cchar}(undef, 1024 * 8) | ||
ierr = GC.@preserve buffer begin | ||
Lib.XPRSlicense(lic, Cstring(pointer(buffer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this buffer might not be needed, it probably came from a copy-paste from the usage below.
path_lic = _get_xpauthpath(; verbose) | ||
touch(path_lic) | ||
lic = Ref{Cint}(1) | ||
# TODO(odow): why do we call XPRSlicense twice? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see the explanation in the license.jl file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find the comments very descriptive haha
No description provided.