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

Cython wrapper build fix #117

Closed
wants to merge 6 commits into from

Conversation

AlexJoz
Copy link
Collaborator

@AlexJoz AlexJoz commented Oct 15, 2017

Hello.
Seems user keys works only for private root repos, so I managed to make it work with user credentials encrypted via travis. Is it possible to make the dedicated user for that purpose?

And the second question, why shouldn't we use Hunter? Maybe we can add eigen via ExternalProject_Add in cmake or smth similar specially for travis build, but does it worth it? Or you just want to cut out Hunter away?

@ibayer
Copy link
Owner

ibayer commented Oct 16, 2017

Is it possible to make the dedicated user for that purpose?

That's a good idea, I have already created one some time ago for this purpose.
I'll send you the credentials via upwork.

And the second question, why shouldn't we use Hunter?

Hunter is a huge help in managing dependencies but not having any dependencies is even better.
The part of the project relevant for the python wrapper (see README Build only fastFM lib) has only EIGEN as dependency (which is header only) and we could for example include EIGEN as submodule or external project (whatever is simpler).

I think dropping Hunter for this part simplifies things without any real downside. Simplicity is very important, people need to understand the project setup quickly to contribute improvements to the project.

@AlexJoz
Copy link
Collaborator Author

AlexJoz commented Oct 17, 2017

Got the credentials for user, but i'm not allowed to make a environment variable in your travis settings.

p.s. if it's hard with travis access rights, I can send you the details, so you add it by yourself (this wont take much time - just apply the command)

@ibayer
Copy link
Owner

ibayer commented Oct 18, 2017

@AlexJoz
Did you check if this works on your fork (you should be able to change travis settings there)?

Yes, please send me step by step instructions (or a pointer to relevant doc) when you are confident that this will work (I have unsuccessfully tried a few things before) and I will do the changes.

@AlexJoz
Copy link
Collaborator Author

AlexJoz commented Oct 18, 2017

Yep, i checked it.
I sent a small instruction as upwork message..

.gitmodules Outdated
@@ -3,5 +3,5 @@
url = https://github.com/ibayer/fastFM-core.git
[submodule "fastFM-core2"]
path = fastFM-core2
url = git@github.com:ibayer/fastFM-core2.git
url = git@github.com:AlexJoz/fastFM-core2.git
Copy link
Owner

@ibayer ibayer Oct 18, 2017

Choose a reason for hiding this comment

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

Probably need to be changed back or?
You can create new branch in my repo and push you changes there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, i'll fix it back, after i can test with your repo

@ibayer
Copy link
Owner

ibayer commented Oct 18, 2017

I have added the CI_TOKEN.

fatal: Authentication failed for 'https://@github.com/AlexJoz/fastFM-core2.git/'

@AlexJoz
Copy link
Collaborator Author

AlexJoz commented Oct 18, 2017

oh, ok. thnx. I'll check )

@AlexJoz
Copy link
Collaborator Author

AlexJoz commented Oct 18, 2017

Hm, we cannot check this in PR anyway, coz https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions

p.s. i have no rights to push to this repo, to check in branch...

@ibayer
Copy link
Owner

ibayer commented Oct 18, 2017

Makes sense I have given you access rights. You can push changes to cython-wrapper branch or I merge this PR first.

@ibayer
Copy link
Owner

ibayer commented Oct 25, 2017

@AlexJoz Can we close this PR?

@AlexJoz
Copy link
Collaborator Author

AlexJoz commented Oct 25, 2017

yep)

@AlexJoz AlexJoz closed this Oct 25, 2017
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