-
Notifications
You must be signed in to change notification settings - Fork 31
Use Resource::Wrangler for handling resource paths #109
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
Conversation
This simplifies the code and removes the necessity to worry over the details of using temporary files for accessing resources.
META6.json
Outdated
@@ -5,7 +5,8 @@ | |||
], | |||
"build-depends": [ | |||
"PathTools", | |||
"JSON::Fast" | |||
"JSON::Fast", | |||
"Resource::Wrangler" |
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.
Could you make this a full dependency: Resource::Wrangler:ver<1.0.2+>:authzef:ab5tract
META6.json
Outdated
@@ -45,5 +46,5 @@ | |||
], | |||
"test-depends": [ | |||
], | |||
"version": "0.2.4" | |||
"version": "0.2.5" |
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 is an App::Mi6 enabled module: on release this will automatically updated.
@@ -1,5 +1,7 @@ | |||
unit module OpenSSL::NativeLib; | |||
|
|||
use Resource::Wrangler; |
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.
Also full dependency please :-)
To allow a release, the Changes file needs an update. |
META6.json
Outdated
@@ -5,7 +5,8 @@ | |||
], | |||
"build-depends": [ | |||
"PathTools", | |||
"JSON::Fast" | |||
"JSON::Fast", | |||
"Resource::Wrangler" |
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 is a regular runtime dependency
As you can see from the CI this isn't working on Windows |
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 previous comments
lib/OpenSSL/NativeLib.rakumod
Outdated
} | ||
|
||
$dll-resource.absolute | ||
load-resource-to-path($resource-name).absolute |
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'm not sure Resource::Wrangler
is ideal to use here if it indeed needs to generate a unique path part every time it is run since that means on Windows we'd constantly be copying the OpenSSL libraries at runtime. In my original code above it would probably have been better to $*HOME
as the prefix instead of $*TMPDIR
to avoid security issues, but either way it has the advantage of putting the file in a deterministic spot so it is ideally only ever copied once.
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.
Both $prefix
and $filename
are available as optional parameters to the load-resource-to-path
sub, so it is still a viable option here.
But for now I'm just trying to understand why the related tests are failing 🙃
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.
FWIW, on Windows $*TMPDIR is already underneath a user's folder.
8b560df
to
c9f7ba5
Compare
c9f7ba5
to
61b175e
Compare
Closing as the current implementation of |
This simplifies the code and removes the necessity to worry over the details of using temporary files for accessing resources.