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.

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

Refactor platform specific process path #4770

Conversation

wmoxam
Copy link
Contributor

@wmoxam wmoxam commented Aug 1, 2017

Moves all the platform specifics for Process. executable_path into it's own namespace.

Similar to work done in #4450 & #4466, proposed in ##4406

end

{% if flag?(:darwin) %}
require "./unix/darwin_process"
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to name the files process_darwin etc. to have those process implementations together in lexical sort.

{% elsif flag?(:linux) %}
require "./unix/linux_process"
{% else %}
# TODO: restrict on flag?(:unix) after crystal > 0.22.0 is released
Copy link
Member

Choose a reason for hiding this comment

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

That TODO is already outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

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.

{% elsif flag?(:linux) %}
require "./unix/linux_process"
{% else %}
# TODO: restrict on flag?(:unix) after crystal > 0.22.0 is released
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -215,7 +215,7 @@ describe Process do
end

describe "find_executable" do
pwd = Process::INITIAL_PWD
pwd = Dir.current
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure: by the time the spec is run, the current directory may have been changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT that doesn't seem to be the case

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @ysbaddaden's point is that thats not something you can rely on.

@@ -0,0 +1,4 @@
lib LibC
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the C header is mach-o/dyld.h so it should match it: c/mach-o/dyld.cr.

@wmoxam wmoxam force-pushed the refactor-platform-specific-process-path branch from 526ad94 to f8eef57 Compare August 17, 2017 01:48

if LibC._NSGetExecutablePath(buf, pointerof(size)) == -1
buf = GC.malloc_atomic(size).as(UInt8*)
return nil if LibC._NSGetExecutablePath(buf, pointerof(size)) == -1
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong call, you're right.

@wmoxam
Copy link
Contributor Author

wmoxam commented Sep 9, 2017

Is this sort of re-factoring still a goal? If so, there are a few other places where this should be done

@mverzilli
Copy link

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!

@sdogruyol
Copy link
Member

Can we merge this?

@ysbaddaden
Copy link
Contributor

There are mixed intents: we still use Crystal::System in master but this PR changed to monkey patching System (but still declares Crystal::System::Process) as per #4906

@wmoxam wmoxam closed this Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants