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

boostrap miniconda directly #40

Merged
merged 1 commit into from Apr 10, 2017

Conversation

jhoblitt
Copy link
Member

@jhoblitt jhoblitt commented Apr 8, 2017

Instead of installing it as an EUPS distrib product. This removes the
need to have a pre-existing python environment from which to get EUPS
running. It also ensures that if miniconda is installed, it will be the
interpreter used by EUPS. # Please enter the commit message for your
changes. Lines starting

@jhoblitt jhoblitt force-pushed the tickets/DM-5126-miniconda branch 4 times, most recently from 0b1d31c to 55d6a4f Compare April 8, 2017 00:26
@jhoblitt jhoblitt requested review from timj and athornton April 8, 2017 00:33
@jhoblitt
Copy link
Member Author

jhoblitt commented Apr 8, 2017

As a side comment, this script could use overall some style cleanup / refactoring but I've tried to keep the number of non-functional changes down in order to not pollute the diff.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I have reservations about the code duplication between this and deploy. Why is this change needed for eups? What problem does the current system have that means that we need to ensure that EUPS uses our installed python? The JIRA ticket does not make a good case.


# By default we use the PATH Python to bootstrap EUPS.
# Set $PYTHON to override this or use the -P command line option.
# $PYTHON is used to install and run EUPS and will not necessarily
Copy link
Member

Choose a reason for hiding this comment

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

The -P option still sets $PYTHON but nothing uses it. Should -P now be setting $EUPS_PYTHON?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like something got lost in a rebase as I had universally fixed this...

if [[ $(uname -s) = Darwin* ]]; then
# run install_name_tool on all of the libpythonX.X.dylib dynamic
# libraries in miniconda
for entry in $prefix/lib/libpython*.dylib; do
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this? It's not in lsstsw and I thought the modern conda python package got this right.

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 -- I was copying what the miniconda2/3 products were doing. I can remove it to see if anything breaks.

MINICONDA2_VERSION=${MINICONDA2_VERSION:-4.2.12.lsst2}
MINICONDA3_VERSION=${MINICONDA3_VERSION:-4.2.12.lsst2}
MINICONDA_VERSION=${MINICONDA_VERSION:-4.2.12}
LSSTSW_REF=${LSSTSW_REF:-7c8e67}
Copy link
Member

Choose a reason for hiding this comment

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

Can there be a comment here explaining that this ref is used to define the pinned package versions for conda? The main problem we have is that v13.0 of the stack wants different pinned versions to v12.0 and once you've installed a v13.0 stack your chances of building a v12.0 in the same tree are slim to none.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@jhoblitt jhoblitt force-pushed the tickets/DM-5126-miniconda branch 4 times, most recently from 71128a4 to 33bdc8f Compare April 10, 2017 21:52
Instead of installing it as an EUPS distrib product.  This removes the
need to have a pre-existing python environment from which to get EUPS
running.  It also ensures that if miniconda is installed, it will be the
interpreter used by EUPS.  # Please enter the commit message for your
changes. Lines starting
@jhoblitt jhoblitt merged commit 0e16c30 into lsst:master Apr 10, 2017
@jhoblitt jhoblitt deleted the tickets/DM-5126-miniconda branch April 10, 2017 22:00
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.

None yet

2 participants