Skip to content

Use Process.run instead of command operator#16780

Open
ysbaddaden wants to merge 3 commits intocrystal-lang:masterfrom
ysbaddaden:refactor/use-process-run-instead-of-command-operator
Open

Use Process.run instead of command operator#16780
ysbaddaden wants to merge 3 commits intocrystal-lang:masterfrom
ysbaddaden:refactor/use-process-run-instead-of-command-operator

Conversation

@ysbaddaden
Copy link
Copy Markdown
Collaborator

@ysbaddaden ysbaddaden commented Mar 24, 2026

Allows to directly pass args and call the executable instead of quoting each arg and building a shell command indirection.

Ref #16614.
Extracted from #16630.

Copy link
Copy Markdown
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

question: Any reason for using Process(command, args) instead of the new Process.run(args) API? I think we should rather adapt the latter.

@ysbaddaden ysbaddaden changed the title Use Process.run instead of command operator Use Process.run instead of command operator Mar 24, 2026
@ysbaddaden
Copy link
Copy Markdown
Collaborator Author

None. It wasn't implemented when I wrote the original patch.

BTW: changing to the other syntax, we should just have a *args overload. Forcing every call site to wrap in a %W, Array or Tuple is just noise.

class Process
  def self.run(*args : String, **kwargs)
    run(args, **kwargs)
  end
end

@straight-shoota
Copy link
Copy Markdown
Member

Let's add that to RFC 25!

@ysbaddaden
Copy link
Copy Markdown
Collaborator Author

Why would the aarch64 CI fail?

@ysbaddaden
Copy link
Copy Markdown
Collaborator Author

I reverted the change to Process.run(args) because it fails with a freshly built compiler:

$ make
$ bin/crystal spec spec/compiler/loader/unix_spec.cr
In src/crystal/system/unix/process.cr:314:21

 314 | {pathname, argv.to_unsafe}
                       ^--------
Error: undefined method 'to_unsafe' for Tuple(Pointer(UInt8), Pointer(UInt8), Pointer(UInt8), Pointer(UInt8), Pointer(UInt8), Pointer(UInt8))

I guess the new API doesn't work with anything but Array?

@straight-shoota
Copy link
Copy Markdown
Member

Yeah. Looks like we're not actually testing that the Enumerable(String) restriction is fully supported. The implementation seems to expect Array(String). 🙈

Allows to directly pass args and call the executable directly instead of
quoting each arg and building a shell command indirection.
@ysbaddaden ysbaddaden force-pushed the refactor/use-process-run-instead-of-command-operator branch from b830a5a to 3d7fc43 Compare April 3, 2026 13:06
@ysbaddaden
Copy link
Copy Markdown
Collaborator Author

I removed the revert commit and replaced the Tuple an Array literal so we have Process.run [...] that's a bit more clear than Process.run({...}). I'd have merely dropped the tuple, but Process.run(*args) isn't available (yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants