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

Enable codeintel in the IDE #98

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Enable codeintel in the IDE #98

wants to merge 3 commits into from

Conversation

etiennewan
Copy link
Member

I experienced a popup in the IDE asking if I wanted to install and enable codeintel.
This PR is based on https://github.com/c9/c9.ide.language.codeintel and on https://github.com/c9/c9.ide.language.codeintel/blob/435e0840663ee5e7fecaabfd3daa865eb2113e4c/codeintel.js#L149.

Copy link
Member

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

🎉 Thanks a lot for this pull request! 🎉 😄

I experienced a popup in the IDE asking if I wanted to install and enable codeintel.

Oh oh, this sounds like a bug. Are you sure that codeintel isn't already installed? I think it's supposed to be a "core plugin" of Cloud9, i.e. pre-installed with Cloud9 SDK.

Do you have a way to systematically trigger this pop-up, in order for me to reproduce this issue? I have never seen it myself, nor have I heard other users mentioning it.

Maybe you were trying to unlock some advanced Cloud9 feature? If so, I'd be super happy to help enable whatever you're trying to achieve 😄

# Install dependencies to enable codeintel (https://github.com/c9/c9.ide.language.codeintel)
RUN easy_install pip
&& pip install -U pip
&& pip install -U virtualenv
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This will no longer be necessary when #97 is merged (it will already have the latest pip and virtualenv).

Copy link
Member Author

Choose a reason for hiding this comment

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

removed considering #97

&& virtualenv --python=python2 $HOME/.c9/python2
&& source $HOME/.c9/python2/bin/activate
&& apt-get update
&& apt-get install python-dev
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This shouldn't be necessary. If you need python-dev to be installed in the ubuntu-dev image, please add it to the list of installed packages at the top of this dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

moved python-dev to the top of file, I think it is indeed needed, but I'm not 100% sure

&& pip download -d /tmp/codeintel codeintel==0.9.3
&& cd /tmp/codeintel
&& tar xf CodeIntel-0.9.3.tar.gz
&& mv CodeIntel-0.9.3/SilverCity CodeIntel-0.9.3/silvercity
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, why are you renaming SilverCity to silvercity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know much, I have to check https://github.com/c9/c9.ide.language.codeintel, that's what they advocate to do

&& tar xf CodeIntel-0.9.3.tar.gz
&& mv CodeIntel-0.9.3/SilverCity CodeIntel-0.9.3/silvercity
&& tar czf CodeIntel-0.9.3.tar.gz CodeIntel-0.9.3
&& pip install -U --no-index --find-links=/tmp/codeintel codeintel
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please delete the temporary /tmp/codeintel folder once you no longer need it. (And if some files need to be kept around for the installation to work, please move them somewhere under /home/user, e.g. as dotfiles or even better close to where it will be used, e.g. under a new folder in /home/user/.c9 as we do with noVNC).

Copy link
Member Author

Choose a reason for hiding this comment

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

They advocate to work under /tmp/ for this part, I assume pip does all the work, and nothing is necessary afterwards. done

Copy link
Member

Choose a reason for hiding this comment

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

All these steps look like a very complicated way to do pip install codeintel==0.9.3. Wouldn't such a simple command work as well?

@@ -71,6 +71,21 @@ module.exports = function (options) {

// Use a longer scrollback for the Terminal.
p.settings.user.terminal['@scrollback'] = 10000;

if (!p.settings.project) {
p.settings.user = {};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You probably mean to create p.settings.project here, as opposed to p.settings.user.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

}

if (!p.settings.project.codeintel) {
p.settings.user.codeintel = {};
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Same as above, s/user/project/.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@etiennewan
Copy link
Member Author

etiennewan commented Oct 9, 2017

Last commit fixes my mistakes and your nits. This should be working, but better test twice!
I was talking about the popup which is triggered when the IDE thinks you're looking for snippets or autocompletion. One way to trigger it (from the IDE of any actual or fresh-spawned container), is to create and save a PHP file, then type '<?php', then '$'. The popup should show up asking you if you want to install codeintel.

I need more testing, the codeintel server doesn't appear to be running...

Copy link
Member

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Thanks for updating this pull request! 😄

I noticed that the codeintel repository is already checked-out in c9/core/plugins: https://github.com/c9/core/tree/master/plugins/c9.ide.language.codeintel so I find it strange that c9/core/scripts/install-sdk.sh doesn't already install such external dependencies. Oh well, at least we'll be doing that for Janitor containers (thanks!)

@@ -57,6 +57,7 @@ RUN __LLVM_VERSION__="5.0" \
valgrind \
x11vnc \
xvfb \
python-dev \
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please try to keep the list of packages in alphabetical order.

@@ -201,6 +202,18 @@ RUN git clone https://github.com/kanaka/noVNC /home/user/.novnc/ \
&& cd /home/user/.novnc \
&& npm install \
&& node ./utils/use_require.js --as commonjs --with-app

# Install dependencies to enable codeintel (https://github.com/c9/c9.ide.language.codeintel)
RUN virtualenv --python=python2 $HOME/.c9/python2
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please hardcode /home/user instead of using the $HOME variable (just to be extra safe).

Copy link
Member

@jankeromnes jankeromnes Oct 13, 2017

Choose a reason for hiding this comment

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

Nit: All these lines need to be terminated by a \ (otherwise docker build will complain about && not being a valid Dockerfile instruction).

&& tar xf CodeIntel-0.9.3.tar.gz
&& mv CodeIntel-0.9.3/SilverCity CodeIntel-0.9.3/silvercity
&& tar czf CodeIntel-0.9.3.tar.gz CodeIntel-0.9.3
&& pip install -U --no-index --find-links=/tmp/codeintel codeintel
Copy link
Member

Choose a reason for hiding this comment

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

All these steps look like a very complicated way to do pip install codeintel==0.9.3. Wouldn't such a simple command work as well?

@@ -71,6 +71,21 @@ module.exports = function (options) {

// Use a longer scrollback for the Terminal.
p.settings.user.terminal['@scrollback'] = 10000;

if (!p.settings.project) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please add a comment similar to "Tweak the default user settings" above (ideally with a link to a file or list of every available project setting).

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.

2 participants