Skip to content

Conversation

@ekohl
Copy link
Contributor

@ekohl ekohl commented Jan 6, 2026

RuboCop rightfully complains about using open() to spawn a process. This uses the way simpler IO.popen() method.

I have no idea if this is going to break or not and whether it's an API breakage.

open("| #{command_str} 2>&1") do |pipe| # rubocop:disable Security/Open
yield pipe
end
english_env = ENV.merge({ 'LANG' => 'C', 'LC_ALL' => 'C' })

Choose a reason for hiding this comment

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

I don't know ruby but should that be?

Suggested change
english_env = ENV.merge({ 'LANG' => 'C', 'LC_ALL' => 'C' })
english_env = ENV.to_hash.merge({ 'LANG' => 'C', 'LC_ALL' => 'C' })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out ENV is actually an Object and not a Hash. It just looks like one.

I've now simplified it to just be the overrides because the default behavior of spawning a process is to inherit all the other vars. So I can't think of why the whole merging was needed to begin with.

@ekohl ekohl force-pushed the avoid-open-to-exec branch 3 times, most recently from 6f1ca15 to 7d9a6e0 Compare January 6, 2026 22:12
@ekohl
Copy link
Contributor Author

ekohl commented Jan 6, 2026

I know this is incomplete and some tests still fail, but it's a start.

RuboCop rightfully complains about using open() to spawn a process. This
uses the way simpler IO.popen() method.
@ekohl ekohl force-pushed the avoid-open-to-exec branch from 7d9a6e0 to fd54e75 Compare January 6, 2026 22:37
@ekohl ekohl marked this pull request as ready for review January 6, 2026 22:41
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.

2 participants