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

Add Warning when Conda is installed #2596

Merged
merged 2 commits into from Jul 8, 2016

Conversation

Projects
None yet
4 participants
@remimarenco
Copy link
Contributor

commented Jul 7, 2016

because we skip the virtualenv activation.

Here is a potential help for people having Conda installed, but then have issues because of missing dependencies (#1633).

A few problematics still exist:

  • Is it normal to have failed dependencies? Do we expect people to have them because they installed Conda?
  • Will people be able to see the Warning? The run.sh / common_startup.sh are pretty verbose. I don't expect everybody to check every line of code trying to find a Warning. So what can we do to help people find this errors, in your opinion?

xref: #1633 (comment)

Add Warning when Conda is installed because we skip the virtualenv cr…
…eation

Fix the case where we could have conda not installed but not setting VirtualEnv still (and have this non relevant warning

@galaxybot galaxybot added the triage label Jul 7, 2016

@galaxybot galaxybot added this to the 16.07 milestone Jul 7, 2016

@@ -9,6 +9,7 @@ done
# Conda Python is in use, do not use virtualenv
if python -V 2>&1 | grep -q -e 'Anaconda' -e 'Continuum Analytics' ; then
SET_VENV=0
CONDA_ALREADY_INSTALLED=1

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 7, 2016

Member

I think you can just print the message here, nothing changes the value of SET_VENV after this.

This comment has been minimized.

Copy link
@remimarenco

remimarenco Jul 7, 2016

Author Contributor

Nicola, thanks for taking the time to review this :).

Yes I hesitated, and I first did put in there. But then I saw the verbosity and thought it would be nice to put this as far as possible in the logs to have more chances for people to see it. Plus, I thought that it could be an open way for people to add more stuff to handle the case of Conda being already installed.

If you think it is not useful, and others too, I can totally move it back there and remove the extra variable.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 7, 2016

Member

No strong opinion about this, let's see if others have something to say.

This comment has been minimized.

Copy link
@dannon

dannon Jul 7, 2016

Member

Seems reasonable to me to echo the statement at the point of the behavioral branch, so, here.

This comment has been minimized.

Copy link
@remimarenco

remimarenco Jul 7, 2016

Author Contributor

Well, the Conda check is done here, but the echo statement is not: You have Conda, but Conda affects VirtualVenv, which is done later on in the code. So if we move the echo here, we would be printing something that is not a concern...yet, right?

This comment has been minimized.

Copy link
@dannon

dannon Jul 7, 2016

Member

I am actually agreeing with the way you wrote it originally, sorry that was not clear. When I replied to the comment I thought it was attached to that other block :)

@@ -127,6 +128,8 @@ if [ $SET_VENV -eq 1 ]; then
echo "ERROR: A virtualenv cannot be found. Please create a virtualenv in $GALAXY_VIRTUAL_ENV, or activate one."
exit 1
fi
elif [ $SET_VENV -eq -0 -a $CONDA_ALREADY_INSTALLED -eq 1 ]; then
echo -e "\e[33mWarning: You have Conda installed. Skipping virtualenv creation. This could cause missing dependencies.\e[0m"

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 7, 2016

Member

s/creation/activation/

This comment has been minimized.

Copy link
@remimarenco

remimarenco Jul 7, 2016

Author Contributor

Fixed :)

@dannon dannon merged commit c0101e0 into galaxyproject:dev Jul 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.