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.

Sign up for GitHub

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

Jump to bottom

feat: replace symlink with simple shell wrapper #346

Closed
wants to merge 7 commits into from
Closed

feat: replace symlink with simple shell wrapper #346

wants to merge 7 commits into from

Conversation

Chumper
Copy link
Collaborator

@Chumper Chumper commented Mar 18, 2022

Replaces the symlink for tools with a simple shell wrapper

@Chumper
Copy link
Collaborator Author

Chumper commented Mar 18, 2022

Closes #338

viceice
viceice reviewed Mar 19, 2022
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Node tests are failing. 🤔

@Chumper
Copy link
Collaborator Author

Chumper commented Mar 19, 2022

yes, but i don't understand why... I need to dig deeper to understand the problem

@Chumper
Copy link
Collaborator Author

Chumper commented Mar 20, 2022

I am not able to find the error. I suspect due to the shell script, paths have been changed but even a diff of the working and failing run does not bring me closer to a solution.

@Chumper
Copy link
Collaborator Author

Chumper commented Mar 20, 2022

Apparently with ln npm gets installed in /home/user/npm/8.0.0, with the shell script in /home/user/.npm-global/bin/npm

@viceice viceice linked an issue Mar 25, 2022 that may be closed by this pull request
User installs should be able to overwrite tool symlinks created by root installs #338
Closed
@viceice viceice mentioned this pull request Mar 25, 2022
Merged
@viceice viceice changed the title refactor: replace symlink with simple shell wrapper feat: replace symlink with simple shell wrapper Mar 25, 2022
@viceice viceice marked this pull request as draft March 25, 2022 14:23
@viceice
Copy link
Member

viceice commented Mar 25, 2022

We should not change link_wrapper, instead we should move all tools to shell_wrapper and v2 tools.

@Chumper Chumper closed this Apr 2, 2022
@Chumper Chumper deleted the refactor/symlinkToShell branch April 2, 2022 11:49
@florianklumb
Copy link

Any verdict on when this will be deployed (into Docker image)?

@Chumper
Copy link
Collaborator Author

Chumper commented Apr 28, 2022

We opted for refactoring all tool afaik instead of replacing this for all tools at once.

@viceice
Copy link
Member

viceice commented Apr 28, 2022

@florianklumb So you have issues? PLease open a discussion, so we can checkout your usecase. And then we can open an issue if there is a feature request or bug

@florianklumb florianklumb mentioned this pull request Apr 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viceice viceice viceice left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

User installs should be able to overwrite tool symlinks created by root installs
3 participants
@Chumper @viceice @florianklumb