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

Remove unneeded call to ls in nvm_arch #3012

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Remove unneeded call to ls in nvm_arch #3012

merged 1 commit into from
Jan 26, 2023

Conversation

signed-log
Copy link
Contributor

Remove the call to ls that was used to determine the symlink
destination

Reasoning :

  • od resolves symlink itself due to the use of fopen
  • Prevent the behaviour of od which will hang if the filename is
    empty (i.e. /sbin/init missing) as it will be waiting for stdin
    compared to quitting with error if the file just doesn't exist

Closes #3006

Remove the call to `ls` that was used to determine the symlink
destination

Reasoning :

* `od` resolves symlink itself due to the use of `fopen`
* Prevent the behaviour of `od` which will hang if the filename is
  empty (i.e. `/sbin/init` missing) as it will be waiting for `stdin`
  compared to quitting with error if the file just doesn't exist

Fixes #3006
nvm.sh Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jan 23, 2023

Is there a way to write a test for this?

@signed-log
Copy link
Contributor Author

Don't think so

Maybe, to run an install on a /sbin/init-less device like a Docker container

@ljharb
Copy link
Member

ljharb commented Jan 26, 2023

Could we temporarily move /sbin/init in the test setup, and then restore it in a teardown?

@signed-log
Copy link
Contributor Author

signed-log commented Jan 26, 2023

Well, /sbim/init is a symlink, so we could unlink it, do the test and relink it

I wouldn't do it though, It's unknown how much breakage can be done by doing that, rendering the test unreliable at best

I haven't looked at the test infra here, but using a docker container seems to be by far the easiest

@ljharb
Copy link
Member

ljharb commented Jan 26, 2023

Gotcha - in that case let's merge this without a test. Migrating the tests to use Docker is a good idea, but it'd be a massive project.

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.

[Suspected regression] Unable to binary install on zsh
2 participants