Refactor platform specific process path #4770
Conversation
src/crystal/system/process.cr
Outdated
| end | ||
|
|
||
| {% if flag?(:darwin) %} | ||
| require "./unix/darwin_process" |
There was a problem hiding this comment.
It would be better to name the files process_darwin etc. to have those process implementations together in lexical sort.
src/crystal/system/process.cr
Outdated
| {% elsif flag?(:linux) %} | ||
| require "./unix/linux_process" | ||
| {% else %} | ||
| # TODO: restrict on flag?(:unix) after crystal > 0.22.0 is released |
There was a problem hiding this comment.
That TODO is already outdated.
There was a problem hiding this comment.
Actually the TODO should be removed and the else branch implement or require the generic "find executable in PATH" solution, which is valid for both UNIX and Windows for example.
There was a problem hiding this comment.
Thanks, it indeed looks better!
Yet, when introducing the Crystal::System namespace, I purposefully created generic windows and unix subfolders only, in order to avoid freebsd or darwin specifics.
But having different, per-platform specific implementations inside a single generic unix folder feels wrong to the initial idea. It calls for having a darwin, freebsd and linux folders, after all, since there are some platform differences further than POSIX vs Windows.
That means unix/getrandom.cr should be moved under linux/ too.
src/crystal/system/process.cr
Outdated
| {% elsif flag?(:linux) %} | ||
| require "./unix/linux_process" | ||
| {% else %} | ||
| # TODO: restrict on flag?(:unix) after crystal > 0.22.0 is released |
There was a problem hiding this comment.
Actually the TODO should be removed and the else branch implement or require the generic "find executable in PATH" solution, which is valid for both UNIX and Windows for example.
spec/std/process_spec.cr
Outdated
| @@ -215,7 +215,7 @@ describe Process do | |||
| end | |||
|
|
|||
| describe "find_executable" do | |||
| pwd = Process::INITIAL_PWD | |||
| pwd = Dir.current | |||
There was a problem hiding this comment.
I'm not sure: by the time the spec is run, the current directory may have been changed.
There was a problem hiding this comment.
AFAICT that doesn't seem to be the case
There was a problem hiding this comment.
I think @ysbaddaden's point is that thats not something you can rely on.
| @@ -0,0 +1,4 @@ | |||
| lib LibC | |||
There was a problem hiding this comment.
I believe the C header is mach-o/dyld.h so it should match it: c/mach-o/dyld.cr.
wmoxam
force-pushed
the
refactor-platform-specific-process-path
branch
from
526ad94 to
f8eef57
Compare
…ctory may have been changed
|
|
||
| if LibC._NSGetExecutablePath(buf, pointerof(size)) == -1 | ||
| buf = GC.malloc_atomic(size).as(UInt8*) | ||
| return nil if LibC._NSGetExecutablePath(buf, pointerof(size)) == -1 |
There was a problem hiding this comment.
This looks like repeated call to LibC._NSGetExecutablePath(buf, pointerof(size)). Would it be possible to save result of previous call in a variable and use it here?
There was a problem hiding this comment.
No. The first call will report an error and update size to be the actual path length, when the default, arbitrary-sized, buffer is too small. We must make a second call with a larger buffer to get the path.
|
Is this sort of re-factoring still a goal? If so, there are a few other places where this should be done |
|
It still is a goal, but after some initial commits the discussion has been revisited in #4906. Would be good if you could check it out and leave your impressions as well! |
|
Can we merge this? |
|
There are mixed intents: we still use |
Moves all the platform specifics for
Process. executable_pathinto it's own namespace.Similar to work done in #4450 & #4466, proposed in ##4406