nvm-sh / nvm

Node Version Manager - POSIX-compliant bash script to manage multiple active node.js versions
MIT License
80.18k stars 8.02k forks source link

Should export functions to the shell #806

Open leehambley opened 9 years ago

leehambley commented 9 years ago

NVM suffers from the following:

    $ tail -n +1 entrypoint.sh childscript.sh
    ==> entrypoint.sh <==
    #!/bin/bash
    function inheritedfunction() {
      echo "I am an inherited function"
    }
    exec ./childscript.sh
    ==> childscript.sh <==
    #!/bin/bash
    function localfunction() {
      echo "I am a local function"
    }
    localfunction
    inheritedfunction
    $ ./entrypoint.sh
    I am a local function
    ./childscript.sh: line 8: inheritedfunction: command not found

Essentially if you exec (either explicitly with exec ./childscript or implicitly by ./childscript) then NVM becomes unavailable.

gvm and rbenv solve this by having a shell shim that modifies the path and making sure that the binaries are real binaries, really on the $PATH.

Short term suggestion would be to export -f every_function_in_nvm (it's a long list):

+  export -f nvm
+  export -f nvm_add_iojs_prefix
+  export -f nvm_alias
+  export -f nvm_alias_path
+  export -f nvm_binary_available
+  export -f nvm_checksum
+  export -f nvm_download
+  export -f nvm_ensure_default_set
+  export -f nvm_ensure_version_installed
+  export -f nvm_ensure_version_prefix
+  export -f nvm_find_nvmrc
+  export -f nvm_find_up
+  export -f nvm_format_version
+  export -f nvm_get_arch
+  export -f nvm_get_latest
+  export -f nvm_get_os
+  export -f nvm_has
+  export -f nvm_has_system_iojs
+  export -f nvm_has_system_node
+  export -f nvm_install_iojs_binary
+  export -f nvm_install_node_binary
+  export -f nvm_install_node_source
+  export -f nvm_iojs_prefix
+  export -f nvm_is_alias
+  export -f nvm_is_iojs_version
+  export -f nvm_is_valid_version
+  export -f nvm_ls
+  export -f nvm_ls_current
+  export -f nvm_ls_remote
+  export -f nvm_ls_remote_iojs
+  export -f nvm_ls_remote_iojs_org
+  export -f nvm_match_version
+  export -f nvm_node_prefix
+  export -f nvm_normalize_version
+  export -f nvm_npm_global_modules
+  export -f nvm_num_version_groups
+  export -f nvm_prepend_path
+  export -f nvm_print_implicit_alias
+  export -f nvm_print_npm_version
+  export -f nvm_print_versions
+  export -f nvm_rc_version
+  export -f nvm_remote_version
+  export -f nvm_remote_versions
+  export -f nvm_resolve_alias
+  export -f nvm_resolve_local_alias
+  export -f nvm_strip_iojs_prefix
+  export -f nvm_strip_path
+  export -f nvm_supports_source_options
+  export -f nvm_tree_contains_path
+  export -f nvm_validate_implicit_alias
+  export -f nvm_version
+  export -f nvm_version_dir
+  export -f nvm_version_greater
+  export -f nvm_version_greater_than_or_equal_to
+  export -f nvm_version_path
+}

But, with this childscript is able to at least run nvm. Unfortunately this means that $ env becomes essentially useless, as this export -f is a bashism which exports the function body as an environment variable with the same name as the function to allow it to pass through man (3) execv

ljharb commented 9 years ago

The suggestion to split things up into binaries is in #337.

I wouldn't necessarily say nvm "suffers" here - you can also just rerun the source line (. $NVM_DIR/nvm.sh) and that should cover you without having to export each function, right?

leehambley commented 9 years ago

The problem is we're building a CI, and we setup an environment (environment includes nvm, gvm, rbenv, rvm, etc) and then we exec ./the/user/test/script… and we wanted to give them the 100% clean "just run node" experience, if we have to tell them to start sourcing random dot-files, then it takes the shine off the user experience a lot.

I'd be more than happy to let you close this and we can pick up in #337.

leehambley commented 9 years ago

Having just read the arguments in #337 it seems people are largely against changing, fear of breaking changes, and how rbenv's shim approach can cause issues (unfortunately two linked tweets are protected/deleted) so I'm not sure what issues that might be, but it's certainly non-trivial.

ljharb commented 9 years ago

Right - I'm fine with change, but I'd prefer avoiding breaking changes. The shim approach of rvm and rbenv are very intrusive, and that makes testing and support much harder as well.

Why do you need nvm at all in the exec'd shell? Can you not just do nvm use node first, and then export the PATH down?

leehambley commented 9 years ago

We rely on the default, so we don't "nvm use" anything, maybe that would be a workaround.

Regarding the breaking change from the shim, I've always found rbenv's shim approach to be really, really neat - but I suppose it depends a little bit on the entry point, there's another thing here though, what's to stop nvm itself being on the path, working as it does now, but not being a shell function, rather something you can really execute?

ljharb commented 9 years ago

If that was the case, nvm.sh would have to be parsed on every invocation of nvm - unless all of its other constituent parts were also separate executables.

leehambley commented 9 years ago

If that was the case, nvm.sh would have to be parsed on every invocation of nvm - unless all of its other constituent parts were also separate executables.

This is how rbenv works. That part at least seems to work pretty well, although admittedly the shims are a bit of a mess at times (I hated rbenv rehash)