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

JHipster should not install tabtab completions automatically #6162

Closed
1 task done
gmarziou opened this issue Jul 23, 2017 · 21 comments
Closed
1 task done

JHipster should not install tabtab completions automatically #6162

gmarziou opened this issue Jul 23, 2017 · 21 comments

Comments

@gmarziou
Copy link
Contributor

gmarziou commented Jul 23, 2017

Overview of the issue

JHipster should not install tabtab completions automatically, it should ask permission to the user.

Motivation for or Use Case

It's been a long time since I did run jhipster on Windows, in fact it was before jhipster cli was introduced and today I tried and I could not get it working.

One reason why Windows is a hard platform is because our users have many shells: cmd.exe, powershell and git bash are the most common ones. Cygwin can also be found.

So each time we mess with shell on Windows, we can be sure it won't work for some users.

Currently, my problem is with tabtab (which does not work on Windows or may work with bash): it is installed by generator-jhipster using install script in package.json but no uninstall script.

Tabtab writes to a different global location based on type of shell it has detected, surprisingly this includes locations that require sudo rights according to their doc (I did not check) : /usr/local/share/zsh/site-functions, /usr/share/bash-completion/completions or /etc/bash_completion.d.

I think JHipster should not require sudo rights to install.

This forced installation creates another potential issue: it's not versioned, tabtab writes stuff in a global location so if you have projects using different versions of jhipster, all of them will get completions from latest installation.

"jhipster upgrade" installs locally 2 versions of jhipster but we can't tell it not to run tabtab, so a local installation results in global impact which is bad per se and in my particular case results in a failure.

upgrade failing due to tabtab
jhipster upgrade
Executing jhipster:upgrade
Welcome to the JHipster Upgrade Sub-Generator
This will upgrade your current application codebase to the latest JHipster version
Looking for latest generator-jhipster version...
New generator-jhipster version found: 4.6.2
Git repository detected
Created branch jhipster_upgrade
Cleaned up directory
Installing JHipster 4.6.1 locally
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: �[31m�[1mERROR!�[22m�[39m Something went wrong while installing the JHipster generator! yarn add v0.27.5
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.
error E:\projets\issues\upgrade\node_modules\generator-jhipster: Command failed.
Exit code: 1
Command: C:\WINDOWS\system32\cmd.exe
Arguments: /d /s /c tabtab install --name jhipster --auto
Directory: E:\projets\issues\upgrade\node_modules\generator-jhipster
Output:
module.js:471
throw err;
^

Error: Cannot find module 'E:\projets\issues\upgrade\node_modules\issues\upgrade\node_modules\tabtab\bin\tabtab'
at Function.Module._resolveFilename (module.js:469:15)
at Function.Module._load (module.js:417:25)
at Module.runMain (module.js:604:10)


It's not ideal also for continuous integration either when you could have concurrency issues between different builds.

Reproduce the error
Related issues
Suggest a Fix

Get rid of tabtab in install script and prompt the user for installing it or document it as a pro tip.

JHipster Version(s)

4.6.2

JHipster configuration
Entity configuration(s) entityName.json files generated in the .jhipster directory
Browsers and Operating System
  • Checking this box is mandatory (this is just to show you read everything)
@pascalgrimaud
Copy link
Member

Although there is an error message, the generator-jhipster should be installed.
But, I agree with you @gmarziou, if tabtab causes trouble, we can try to propose a way to install it separately.

Anyway, jhipster-oh-my-zsh is the best 😛

@gmarziou
Copy link
Contributor Author

Installation works fine but upgrade fails.

@jdubois
Copy link
Member

jdubois commented Jul 23, 2017

  • we already had a similar issue with someone on Windows or Mac OS X who was upset we modified his .profile (that's what tabtab does to install autocompletion)
  • Angular CLI has a specific command to install autocompletion

-> we could have a specific sub-generator that installs autocompletion

The only downside is that it goes against our "great developer experience" goal.

@gmarziou
Copy link
Contributor Author

First great user experience is that it works ;)

@gmarziou
Copy link
Contributor Author

gmarziou commented Jul 23, 2017

could not find the issue with this user about .profile but I found this related issue: #6089

@deepu105
Copy link
Member

@gmarziou @jdubois @pascalgrimaud TabTab doesnt support windows as far as I know so we have few options to consider. (Not in any order)

  1. We could just disable auto-completion support all together :(
  2. We could ask user permission before installing TabTab (currently I set it to auto, there is also option to prompt user)
  3. We could load completion manually per session (https://github.com/mklabs/node-tabtab#manual-installation)
    4.We could disable installation in windows alone

So please comment on what is more preferable

@PierreBesson
Copy link
Contributor

PierreBesson commented Jul 23, 2017

@deepu105, Personally I'm not a big fan of tabtab and it's approach to magically configure things (modifying personal conf files...). Although it always worked for me with nothing to do so it is still kinda cool...
I suggest we move to a different approach which is writing completion scripts ourselves and creating a new completion cli command. So running jhipster completion bash and jhipster completion zsh will output the completion script.

So people need to add the following line to their bashrc/zshrc: source <(jhipster completion bash). This is the approach used by kubectl and I really like it. The shell is a little slower to startup but you are guaranteed that completions correspond to your current cli version. Also it will let us do much smarter completions.

@gmarziou
Copy link
Contributor Author

gmarziou commented Jul 23, 2017

@deepu105 I'm not sure option 2 would work with upgrade, option 4 could work but it does not solve the other issues I mentioned.

I'm in favor of letting the user decide what to do with her/his files, like baking them up before running sub module, so I like @PierreBesson idea.

In the same idea, I think the upgrade generator should warn the user that it's going to make some changes in her/his local git repo and that it would be a good idea to push local branches to server before starting the upgrade.

@deepu105
Copy link
Member

@PierreBesson I like your idea but then the problem is it needs manual steps to make it work and hence mostly will be used only by us and other experienced people. I would atleast like to prompt the user to install completion during installation otherwise I'm afraid that no body will actually do it and use it. It will be like our fish/zsh plugins I guess its used only by our core team mostly (of course just a guess based on queries we get for them)

@gmarziou
Copy link
Contributor Author

Maybe a slight variation of @PierreBesson's idea: we could a have a new sub generator dedicated to all potential helpers (same as what ci-cd does in supporting several tools) and ask a question in the main generator: do you want to improve your development environment?
It is vague enough to include shell completion, IDEs, editors....

I have thought of naming it: "environment", "boost" or "productivity".

@PierreBesson
Copy link
Contributor

Maybe we can just print instructions to setup completion after the generator install. If it is just a matter of adding one line to .bashrc/.zshrc I'm sure many people will do it.
We can also propose to install it automatically but we will need to code something for shell detection...

I'm currently hacking on some auto complete scripts... Currently my main issue with our current auto complete is that it's not great, for example it can't auto complete file names when running jhipster import-jdl.

gmarziou added a commit to gmarziou/generator-jhipster that referenced this issue Jul 29, 2017
Npm scrpits of generator-jhipster are useful for global installation but they are not required in teh context of an upgrade where the focus is on code and depenencies.

These scripts can make the upgrade fail for bad reasons llike issue jhipster#6162 about tabtab. 

This change add `--ignore-scripts`option  to yarn or npm install during upgrade.
@gmarziou
Copy link
Contributor Author

gmarziou commented Aug 5, 2017

Until we take a long term decision we need to disable tabtab forced installation, it just blocks some users and using --ignore-scripts is only a workaround.

The 2 PR #6180 and #6202 are worth a patch release soon I think.

@deepu105 deepu105 reopened this Aug 5, 2017
@deepu105
Copy link
Member

deepu105 commented Aug 5, 2017

We could also look at how npm does the autocompletion. As they add anything to the . files

@deepu105
Copy link
Member

deepu105 commented Aug 5, 2017

Ahh, ok tabtab does the same actually, we just need to use it properly

@deepu105
Copy link
Member

deepu105 commented Aug 5, 2017

Dis some digging so the below enables completion on current shell session for jhipster, but for this to work you need tabtab installed globally with npm i -g tabtab or similar yarn cmd. There is a minor issue of a : at the end though, may be we could find a way to trigger this with a specific command or with some shell magic(which is not my forte)
. <(tabtab install --stdout --name jhipster)

@stieler-it
Copy link
Contributor

After upgrading from generator-jhipster 4.5.3 to 4.6.2 we cannot build the prod profile any more. Reason is that we are using a yarn linked version of the generator and the Windows workaround for symlinks seems to break when Yarn is trying to build generator-jhipster with tabtab. If we remove the --force option from yarn in pom.xml it does not fail on tabtab, but later on the missing sass/vendor folder.

Our workaround:

yarn unlink generator-jhipster
mvn clean package -Pprod
yarn link generator-jhipster

@gmarziou
Copy link
Contributor Author

Do you really mean 4.6.2 or current master?

@stieler-it
Copy link
Contributor

The problem appeared after upgrading to 4.6.2

@gmarziou
Copy link
Contributor Author

gmarziou commented Aug 22, 2017

As a workaround you could try to add --ignore-scripts to yarn options as run by the maven plugin.
4.7.0 will include removal of the tabtab and should be released soon.

@stieler-it
Copy link
Contributor

Thank you, works for us.

pom.xml diff

- <arguments>install --force</arguments>
+ <arguments>install --force --ignore-scripts</arguments>

mvn clean package -Pprod -> success

@gmarziou gmarziou added this to the 4.7.0 milestone Aug 24, 2017
@gmarziou
Copy link
Contributor Author

gmarziou commented Aug 24, 2017

I think we clan close this issue as tabtab has been removed in 4.7.0 and we can open an improvement issue to track how to provide completion in a less intrusive way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants