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

Upgrading installation to setuptools #4

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

Conversation

Dapid
Copy link
Contributor

@Dapid Dapid commented Sep 17, 2015

This should close #3, and along the way, also #2.

I am having some trouble linking to Eigen in my box, here is the log:
https://gist.github.com/Dapid/94cff5f6f3b4124a23da

The problem seems to be lu.

@Dapid
Copy link
Contributor Author

Dapid commented Sep 17, 2015

Travis is using an older Cython that doesn't support explicit relative imports. Upgrading it and retriggering.

@Dapid
Copy link
Contributor Author

Dapid commented Sep 17, 2015

Finally it works! I have bundled a bunch of things in this PR, but it was needed to make it work.

Travis is using Eigen 3.0.5, but the installation fails on Fedora with Eigen 3.2.5, as the log shows. I will go through the release notes and see if there has been any change.

@strohel
Copy link
Owner

strohel commented Sep 17, 2015

Thanks for this pul request, I'll look at the commits more closely as soon as I find some time.

One thing to note now, however: would you mind changing the commit messages to be in simple (not continuous) present tense? i.e. "migrate to setuptools" instead of "migrating to setuptools". This is what git developers themselves use, see https://git.kernel.org/cgit/git/git.git/tree/Documentation/SubmittingPatches?id=HEAD

Also, I would prefer if you squash the commit fixups, i.e. hide the sausage-making. (see also https://sethrobertson.github.io/GitBestPractices/#sausage )

David Menéndez Hurtado added 3 commits September 18, 2015 02:52
Also, running the Cython compilation in parallel,
avoid a Cython bug where prange cannot run inside a context manager,
use print function for Py3 compatibility.

FIX
Apply PEP8-complying spacing between functions
@Dapid
Copy link
Contributor Author

Dapid commented Sep 18, 2015

I have squashed it into three logical commits, that I think will make it easiest to review. All tests pass locally and on Travis.

@strohel
Copy link
Owner

strohel commented Nov 18, 2015

Thanks, @Dapid - sorry for being extremely slow to respond, I've been rather busy lately, you know that.. I'll have a look at the latest version when time permits.

@Dapid
Copy link
Contributor Author

Dapid commented Nov 18, 2015

Don't worry, I understand. Better to get this right.

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.

pip install fails
2 participants