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

[READY] Test python 3.3 on OS X #363

Merged
merged 5 commits into from
Feb 14, 2016

Conversation

puremourning
Copy link
Member

Force the use of dynamically linked python on OS X

On OS X, Python and all of its extension libraries must be built using the same shared python library. If any of them contain a statically linked libpython.a, then the global variable linkage gets broken, and you are presented with an obscure crash. We force users to link against a dynamically linked python.

Fixes ycm-core/YouCompleteMe#18 (comment).

Testing

Pyenv works fine if you specify the --enable-framework at build time (as noted above).

Also, the virtualenv bug is fixed, so bump the version to latest.

Review on Reviewable

@puremourning puremourning changed the title Test python 3.3 on OS X [READY] Test python 3.3 on OS X Feb 11, 2016
@vheon
Copy link
Contributor

vheon commented Feb 11, 2016

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


ci/travis/travis_install.osx.sh, line 8 [r1] (raw file):
personally I find this harder to read; what about making a bash function which wrap the install || outdate || upgrade and then call it as a command one package per line?

function brew_f() {
  brew install $1 || brew outdated $1 || brew upgrade $1
}

brew_f node.js
brew_f go
...
brew_f pyenv

Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Feb 11, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


ci/travis/travis_install.osx.sh, line 8 [r1] (raw file):
is brew install $1 in the function. Is it possible to edit comments on Reviewable?


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


ci/travis/travis_install.osx.sh, line 8 [r1] (raw file):
Because most of this is non data ink. And this requires less typing.

you have to edit your comments on gihub :)

what about this:

# List of homebrew formulae to install in the order they appear"
REQUIREMENTS="node.js
              go
              ninja
              readline
              autoconf
              pkg-config
              openssl
              pyenv"


# install node, go, ninja, pyenv and dependencies
for pkg in $REQUIREMENTS; do
  # install package, or upgrade it if it is already installed
  brew install $pkg || brew outdated $pkg || brew upgrade $pkg
done

Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator

micbou commented Feb 11, 2016

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


build.py, line 159 [r1] (raw file):
initializes instead of initiliases


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions.


build.py, line 159 [r1] (raw file):
oh my. I will never get used to this new world spleling.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Feb 11, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions.


ci/travis/travis_install.osx.sh, line 8 [r1] (raw file):
I like it :)


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.


build.py, line 164 [r1] (raw file):
LOL @ citation needed :)


ci/travis/travis_install.osx.sh, line 14 [r1] (raw file):
I we're using pyenv in OS X as well, why not move both this and the linux pyenv setup into common config in travis_install? I had it there at one point then moved it into the Linux file because the pyenv approach didn't work on OS X. But you've now resolved that. :)

Seems like we should be able to use upstream (that is, non-brew) pyenv on both platforms.


ci/travis/travis_install.osx.sh, line 40 [r1] (raw file):
Why deal with virtualenv at all now that we use pyenv? After pyenv global #.#.# is used, pip installs into the pyenv dirs. No sudo needed.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


ci/travis/travis_install.osx.sh, line 14 [r1] (raw file):
I'll give it a go. I went with home-brew purely because that's the setup that I tested on my own macs (and was confident would work. Again - laziness)


ci/travis/travis_install.osx.sh, line 40 [r1] (raw file):
All excellent points. Truth is just laziness - debugging travis scripts is seriously time consuming because travis is so slow, and I didn't want to have to debug too much stuff.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

OK i have refactored out the pyenv stuff and it is all good ;)

Appveryor is not working, but i don't think i could possibly have broken that :)


Review status: 1 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

Review status: 1 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


ci/travis/travis_install.osx.sh, line 8 [r1] (raw file):
Done.


ci/travis/travis_install.osx.sh, line 14 [r1] (raw file):
Done.


ci/travis/travis_install.osx.sh, line 40 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@homu
Copy link
Contributor

homu commented Feb 12, 2016

☔ The latest upstream changes (presumably f263af2) made this pull request unmergeable. Please resolve the merge conflicts.

@Valloric
Copy link
Member

Thanks for doing this! :)

:lgtm: Not triggering homu because it's saying this has merge conflicts (otherwise I would).


Review status: 1 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member Author

I have rebased on your PR.


Review status: 1 of 5 files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

Valloric added a commit that referenced this pull request Feb 14, 2016
[READY] Test python 3.3 on OS X
@Valloric Valloric merged commit eeefabe into ycm-core:more-py3 Feb 14, 2016
@Valloric
Copy link
Member

Thanks!

@puremourning puremourning deleted the osx-more-py3 branch March 26, 2016 15:12
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.

5 participants