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

Substitute ICU's stringprep for libidn #217

Open
andrewtj opened this Issue Jun 23, 2013 · 35 comments

Comments

Projects
None yet
5 participants
@andrewtj
Copy link
Contributor

commented Jun 23, 2013

I've slapped together a replacement for the LibIDN class built on ICU's stringprep functions. The code is in a gist for the moment as I'm not keen on updating all the samples, docs, etc to use it before knowing whether there's any interest in seeing it merged.

To build it, grab the files from the gist, icu4c-51_2-src.tgz and icudt51l.zip by selecting rfc3491.spp, rfc3920node.spp and rfc3920res.spp from Miscellaneous Data. Then issue make -j 4 or whatever is appropriate for your machine.

To use it, add ICU4XMPPFramework.h, ICU4XMPPFramework.a and libstdc++ to a project and then swap out LibIDN for ICU4XMPPFramework in XMPPJID.m.

It's not been tested much. I've run it with an OS X app and an iOS app in the simulator and that's about it.

Any interest?

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2013

Not a nibble?

@ObjColumnist

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2013

I suppose you will have to sell us the benefits 😄

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2013

I'd rather just rewrite it and get rid of the dependencies... but that's just me ;)

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2013

@ObjColumnist the appeal for me is the licensing terms. If you're happy with libidn's conditions there's no other major benefits. (Well there's this but I haven't verified it myself.)

@galaxas0 No one's stopping you.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2013

Except time itself. ;)

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2013

@galaxas0 That's precisely why I enlisted ICU 😉

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2013

Hah! I see :) I think going with ICU over LibIDN seems like a good idea too.

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2013

To me the ICU License seems like a clear-win over the LGPLv3 (current libidn license) for development on Apple platforms.

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2013

I'm no lawyer but as I see it using LGPL licensed code in an App-store (or signed or statically linked) app either conflicts or brings extra release requirements due to section 4 of the current license and 6 in the old license.

As I don't want to do this sort of thing with the apps I work on (and I'm not sure it's sufficient in any case) I'm going with ICU in place of libidn. Unless there's further correspondence to indicate otherwise in the next day or so, I'll assume there's no interest and close this issue.

@ObjColumnist

This comment has been minimized.

Copy link
Collaborator

commented Jul 5, 2013

@andrewtj I would like to keep this issue open as it is something that needs addressing, but I would also like to explore the possibility of using just Objective-C.

If your going with ICU in your project, that will be great as you will find out if it has any adverse effects.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

I can rewrite a majority of the requirement in ObjC. I'll try that out.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Having a compatibly licensed product to use as a reference implementation should make it easier to reimplement in ObjC. I had flipped through the Unicode docs once before and they're pretty dense.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Aye, then I'll use this ICU rewrite and "fill in the methods" using pure
Objective-C, referencing stringprep along the way.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Cool. When you get some code together I'm happy to take a look and contribute, particularly in terms of writing test code and refactoring for readability and such.

Does ICU have any test code? Else, come to think of it, does libidn? LGPL-licensed tests would be acceptable.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

Great! I hope this PR will be open until then, I'll try and finish this up
today.

I guess I'll use the whole XMPPFramework as test code, haha!

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

@galaxas0 I think it's worth building this in a new repository. This is general purpose code, useful to other ObjC projects XMPP-related and otherwise, and it saves others the effort of extracting it later. We can import it into this repository (or better, link it as a submodule) once it's done.

Let me know if you'd like a hand setting it up this afternoon. I won't be online much this weekend but should have a little time Monday night or Tuesday.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

@andrewtj if one of us sets up a new repo, would you mind committing your work from the gist?

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2013

I'll start up a new repo today then. Just for the string prep class.

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2013

@paulmelnikow Sure, just fork/clone the gist repo (https://gist.github.com/5843559.git). Consider it licensed under the same terms as XMPPFramework (BSD).

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@galaxas0 Did you get to do that?

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

I started work on it, but I'm hopelessly lost trying to "integrate" LibIDN's stringprep functions.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

Maybe it's more effort than it's worth to reimplement it. I'd like to try to integrate ICU on my end and see what that looks like.

To that end I suggest we rename the class from LibIDN to XMPPStringPrep – see my pull request #225 – and then work on plugging in an alternative.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

Yep, I called it XMPPStringPrep as well.

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2013

@galaxas0 forgive the pedantry, but working from LibIDN somewhat implies you're making a derived work… 😉

I'm not sure what the priorities of the project are, but for my $0.02, removing an LGPL'd dependancy is up there and while writing new code to avoid a new dependancy is laudable, there's other areas that could benefit more from some time and attention.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@andrewtj Did you know that iOS and Mac OS ship with a version of ICU? RegexKitLite seems simply to use -licucore to link against it. I've seen some reports that linking to icucore has caused apps to be rejected, but according to the author of RegexKitLite this hasn't been a problem for his users. The library doesn't install with headers, and I haven't determined what version is available, but it might be worth trying.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

That's completely agreeable, but I know next to nothing about stringprep. Once I have some sort of an implementation, I can rewrite it for efficiency :)

@avaidyam

This comment has been minimized.

@paulmelnikow

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@galaxas0 If you're going to adapt existing code you should start with ICU instead of LibIDN.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@paulmelnikow @andrewtj @ObjColumnist I simply suggest we use the built in ICU framework. /usr/lib/libicucore.dylib.

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2013

@paulmelnikow Yes - I'd read scattered references to people getting rejected for using it though. Given ICU's binary compatibility, I'd guess if you stick to the C API it might maybe be okay. I'd rather err on the side of caution though which is why I went down the route of building standalone with a suffix.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@andrewtj Can you modify the gist to support the built-in library? Or should I try this at home first? ;)

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2013

@galaxas0 I'm not inclined to do that. If you strip off the data libraries and grab the profiles from disk you should be right to go.

@avaidyam

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2013

@andrewtj Alright, I'll give it a go myself then!

@andrewtj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2013

I've converted the gist into a full-fledged repo and renamed the class XMPPStringPrep as per #225.

@chrisballinger

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2014

Just found this the other day, would this be useful? https://github.com/wordpress-mobile/NSURL-IDN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.