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

build(docs): don't install NPM global deps if they're already installed #12522

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

agilgur5
Copy link
Member

Motivation

Only install NPM global deps if they're not already installed

  • previously the make commands would unconditionally install these global deps, which was unnecessary & inefficient the vast majority of the time
    • it would reinstall these every single time I ran these commands

Modifications

  • use npm list -g to check if they're installed first

    • this exits with error code 1 if it doesn't exist
  • note: that was the purpose of the /usr/local/bin/ part in the Makefile, but this is not universally installed in the same place

    • can run npm prefix -g and concat /bin/ to that to get the path to your global installs
      • on my Mac with versioned node (using rtx), the path is: /Users/<my-user>/.local/share/rtx/installs/node/20.6.0/bin/
      • in the devcontainer with versioned node (uses nvm), the path is: /usr/local/share/nvm/versions/node/v20.5.1/bin

Verification

  1. Manually ran the commands with the dependency, confirmed it skipped the install
  2. Manually ran the commands without the dependency, confirmed it performed the install

- use `npm list -g` to check if they're installed first
  - this exits with error code 1 if it doesn't exist
  - previously it would unconditionally install, which was unnecessary & inefficient the vast majority of the time

- note: that was the purpose of the `/usr/local/bin/` part in the `Makefile`, but this is not universally installed in the same place
  - can run `npm prefix -g` and concat `/bin/` to that to get the path to your global installs
    - on my Mac with versioned node (using [`rtx`](https://github.com/jdx/rtx)), the path is: `/Users/<my-user>/.local/share/rtx/installs/node/20.6.0/bin/`
    - in the `devcontainer` with versioned node (uses `nvm`), I have: `/usr/local/share/nvm/versions/node/v20.5.1/bin`

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added the area/build Build or GithubAction/CI issues label Jan 14, 2024
@agilgur5
Copy link
Member Author

agilgur5 commented Jan 14, 2024

@isubasinghe this might make the USE_NIX check no longer necessary? If Nix is installing as a global dep, then this should work for it as well. If so, I can remove the checks for these deps (simplifying the Makefile a tiny bit)

@agilgur5 agilgur5 changed the title fix(build): don't install NPM global deps if they're already installed build(docs): don't install NPM global deps if they're already installed Jan 14, 2024
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Jan 14, 2024
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

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

This seems sensible to me

@isubasinghe
Copy link
Member

@agilgur5 The dependency isn't really installed in Nix via a npm install -g equivalent, so npm list -g will fail (I tested this out),
so the Makefile will have to remain as it is.

I don't think this is necessarily a negative though, it does make the Makefile more consistent at least.

@agilgur5
Copy link
Member Author

@agilgur5 The dependency isn't really installed in Nix via a npm install -g equivalent, so npm list -g will fail (I tested this out)

Thanks for checking!
I thought node2nix might create a similar environment or that the Nix shell would register it an equivalent way, especially if it puts it on $PATH for the make commands to work.
But I guess not, here's the line that modifies $PATH. Potentially a limitation of node2nix or how it was configured, since for reproducibility it should behave similarly to npm i -g

@isubasinghe
Copy link
Member

@agilgur5 The dependency isn't really installed in Nix via a npm install -g equivalent, so npm list -g will fail (I tested this out)

Thanks for checking! I thought node2nix might create a similar environment or that the Nix shell would register it an equivalent way, especially if it puts it on $PATH for the make commands to work. But I guess not, here's the line that modifies $PATH. Potentially a limitation of node2nix or how it was configured, since for reproducibility it should behave similarly to npm i -g

No worries, I don't quite know how it works but I believe it might be due to not assuming a global node installation. I think its a node2nix limitation, really I should replace that tool with raw Nix.

@terrytangyuan terrytangyuan merged commit b73c75c into argoproj:main Feb 5, 2024
28 checks passed
@agilgur5 agilgur5 deleted the fix-build-npm-global-deps branch February 5, 2024 18:39
@agilgur5
Copy link
Member Author

agilgur5 commented Feb 5, 2024

No worries, I don't quite know how it works but I believe it might be due to not assuming a global node installation.

Ah that would make sense if so. Ideally these shouldn't be globally installed either -- or any of the build deps for that matter -- but we've still got a fair bit of changes necessary before we can get there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/docs Incorrect, missing, or mistakes in docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants