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

Python-ldap: link against openssl lib to add ldaps:// support #146

Merged
merged 1 commit into from
Oct 3, 2017

Conversation

abretaud
Copy link
Contributor

@abretaud abretaud commented Sep 1, 2017

Here's a patch to (hopefully) fix galaxyproject/galaxy#3178
There was no ldaps:// support in the python-ldap wheel as it was not compiled with openssl support.
I also updated to latest versions.
I tested it on my laptop with some dockers and it seems to work, though I have no way to check if there's the same bug on macosx.
Let's see if it builds here

@nsoranzo
Copy link
Member

nsoranzo commented Sep 1, 2017

Thanks @abretaud, much appreciated!

@natefoo
Copy link
Member

natefoo commented Sep 8, 2017

Thanks @abretaud! I was annoyed by the chdir() bug you found when originally adding python-ldap so I fixed it and submitted a PR against your fork for it, abretaud#2.

I also included a reformatting of the prebuild command. Multiline prebuild commands aren't supported but I tested and it's possible to use the yaml folding operator to turn it in to a single line when the yaml is loaded. This should make it a little easier to see changes in the future.

For anyone using folding in the future, the command still has to be a valid one line shell command when folded, e.g. this works:

for foo in $bar; do
    baz &&
    quux; 
done

because it becomes:

for foo in $bar; do baz && quux; done

but this (valid multiline shell script) does not:

for foo in $bar
do
    baz
    quux
done

because it becomes:

for foo in $bar do baz quux done

@abretaud
Copy link
Contributor Author

Cool, thanks! That's much more readable like this!
Before merging this I'll make more tests as people still have problem with this new wheel in galaxyproject/galaxy#3178
I'll let you know when it's ready

@natefoo
Copy link
Member

natefoo commented Sep 11, 2017

Ok, thanks!

@abretaud
Copy link
Contributor Author

Ok, it was a nightmare of library dependencies and compilation, but I think I managed to write a working recipe.
@natefoo the build fails because gnutls is in tar.xz format, should we add support for lzma in starforge code or find a tar.gz/bz2 file somewhere (I couldn't find one though)?

@natefoo
Copy link
Member

natefoo commented Sep 15, 2017

Yeah, I'll add lzma support.

@natefoo
Copy link
Member

natefoo commented Oct 1, 2017

Fingers crossed this should build now if you merge abretaud#3 and rebase or merge master.

@abretaud
Copy link
Contributor Author

abretaud commented Oct 2, 2017

Green!
I made a quick test, the wheels works fine for me, but maybe we can wait for some feedback on galaxyproject/galaxy#3178 ?
Is it too late to have it in galaxy 17.09?

@nsoranzo
Copy link
Member

nsoranzo commented Oct 2, 2017

Is it too late to have it in galaxy 17.09?

I think that not having SSL support for LDAP can be considered a bug and would approve a PR targeting 17.09.

@natefoo
Copy link
Member

natefoo commented Oct 2, 2017

I agree with @nsoranzo regarding it being a bug.

@abretaud
Copy link
Contributor Author

abretaud commented Oct 3, 2017

Looks like we're ready to merge finally :)

@natefoo
Copy link
Member

natefoo commented Oct 3, 2017

Ok, they are uploaded to wheels.galaxyproject.org. Thanks for all your work (and patience) on this @abretaud!

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.

Issue with Ldap on version 16.07 SERVER_DOWN: {'desc': "Can't contact LDAP server"} however server is working
3 participants