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

Fix issue with homebrew-installed nc taking precedence in $PATH #12366

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vangie
Copy link

@vangie vangie commented Apr 19, 2024

This commit addresses a problem where the nc command installed via homebrew was being used preferentially over the intended version due to its higher precedence in the $PATH environment variable. Adjustments have been made to ensure the script selects the correct nc executable, avoiding conflicts and ensuring consistent behavior across different setups.

$ which nc
/opt/homebrew/bin/nc

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Changes:

  • [...]

Other comments:

...

This commit addresses a problem where the nc command installed via homebrew was being used preferentially over the intended version due to its higher precedence in the $PATH environment variable. Adjustments have been made to ensure the script selects the correct nc executable, avoiding conflicts and ensuring consistent behavior across different setups.
Copy link
Contributor

ohmyzsh bot commented Apr 19, 2024

Bleep bloop. I determined that these users own the modified files: @septs.

@ohmyzsh ohmyzsh bot added the Area: plugin Issue or PR related to a plugin label Apr 19, 2024
Comment on lines +24 to +28
if sys.platform == 'darwin':
# 'nc' in $PATH may be installed by homebrew, if without path
yield "/usr/bin/nc"
else:
yield "nc"
Copy link
Contributor

@septs septs Apr 19, 2024

Choose a reason for hiding this comment

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

hardcoded paths are bad code.

Suggested change
if sys.platform == 'darwin':
# 'nc' in $PATH may be installed by homebrew, if without path
yield "/usr/bin/nc"
else:
yield "nc"
yield os.environ.get("SHELLPROXY_NETCAT_PROGRAM", "nc")

References

Copy link
Author

Choose a reason for hiding this comment

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

/usr/bin/nc is the BSD version pre-installed in MacOS,If users need to configure the system using SHELLPROXY_NETCAT_PROGRAM, then it might require debugging to identify the issue.

yield "/usr/bin/nc"
else:
yield "nc"
if sys.platform in {'linux', 'cygwin', 'darwin'}:
Copy link
Contributor

Choose a reason for hiding this comment

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

plz split to a new pr, a pull request should only handle one thing.

Copy link
Author

Choose a reason for hiding this comment

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

/usr/bin/nc is the BSD version pre-installed in macos, which need need use the same config as linux and cygwin. Therefore, it's all about addressing the issue of macOS not being ready to use out of the box.

@carlosala
Copy link
Member

@septs what do you think about it?

@carlosala carlosala self-assigned this Apr 22, 2024
@septs
Copy link
Contributor

septs commented Apr 22, 2024

@septs what do you think about it?

The author wants to use it out of the box and hard-coded paths for macOS platform

@carlosala
Copy link
Member

carlosala commented Apr 22, 2024

Wouldn't it be a better solution to properly set $PATH to take the nc you prefer? @vangie
Hardcoding path doesn't seem the way to go.

@vangie
Copy link
Author

vangie commented Apr 27, 2024

@carlosala @septs Let me explain, I am a homebrew and oh-my-zsh user. Initially, my machine only had the BSD version of the nc command that comes with the Mac system.

which -a nc
/usr/bin/nc

shell-proxy is one of my favorite plugins, and I use it almost every day. One day, I installed a Homebrew version of nc.Because the Homebrew command in the PATH has a higher priority than the system default path, most of the time, it meets expectations to use the GNU version of commands.

which -a nc
/opt/homebrew/bin/nc
/usr/bin/nc

After some time, I discovered that shell-proxy was no longer working, but I did not realize that it was due to the previous Homebrew installation. with no choice, I became someone who manually exports the HTTP_PROXY setting. Until I could no longer tolerate it, I began to read the source code of shell-proxy, and after painstakingly debugging step by step, I discovered the aforementioned issue.

Setting the environment variable indeed solves my problem, but users similar to me might not infer from the error messages that the issue stems from having two versions of nc , specifically the differences between the BSD and GNU versions, causing the problem.

Of course, if you still believe that setting an environment variable is more appropriate, I can revise this PR (Pull Request) according to the previous suggestions. At the very least, the next time the plugin updates, I won't have to resolve conflicts locally.

@septs
Copy link
Contributor

septs commented Apr 28, 2024

I still can only accept

yield os.environ.get("SHELLPROXY_NETCAT_PROGRAM", "nc")

@carlosala
Copy link
Member

I agree with @septs, we shouldn't pollute with particular OS cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: plugin Issue or PR related to a plugin
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

None yet

3 participants