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

Naming conflict #24

Closed
daurnimator opened this issue Jan 2, 2015 · 18 comments
Closed

Naming conflict #24

daurnimator opened this issue Jan 2, 2015 · 18 comments

Comments

@daurnimator
Copy link

this library has the same 'require' string as lzlib

Installing this module can cause some weird failures around the place, including luarocks, which uses lzlib when installed

Could you rename the shared object lua-zlib gets installed as?

@brimworks
Copy link
Owner

I'm afraid this is a long standing issue :(. I'm not sure how best to solve this issue since it would cause backwards compatibility problems. When I wrote lua-zlib, I didn't know about lzlib. Suggestions and pull requests are welcomed (as long as it doesn't alienate existing users).

@daurnimator
Copy link
Author

I filed the bug here rather than on lzlib, as looking through a few projects, your project was (sadly) the less popular one.

Maybe if you renamed the library to "lua-zlib" but provided a 'zlib.lua' shim for compatability purposes that just contained:

return require "lua-zlib"

@brimworks
Copy link
Owner

Good idea, although I'm not fond of renaming mine to "lua-zlib". How about "compress.zlib" instead? I think the lzlib project is abandoned :(. Do you want to send a pull request for this? If not, I'll see about getting some time to make this adjustment. Also if you have a better naming idea, let me know.

Thanks,
-Brian

@daurnimator
Copy link
Author

How about "compress.zlib" instead?

That doesn't really make sense to me; that sounds like part of a suite of tools for compression; or to something called 'libcompress' or some such.

if you have a better naming idea, let me know.

I'm trying to think of something....

  • "brimworks.zlib"
  • "z"; precedent in -lz linker flag

@daurnimator
Copy link
Author

any ideas here?

@brimworks
Copy link
Owner

Perhaps I should create a superset interface so that any API calls that "look" like lzlib act the same as lzlib. I could simply copy over the lzlib code.

Thoughts?

Thanks,
-Brian

@daurnimator
Copy link
Author

daurnimator commented Oct 3, 2016

I don't know if that's the best idea.... though perhaps it provides us a way out: lzlib isn't maintained as far as I know; so adding a backwards compatible API, then asking the lua community to essentially deprecate lzlib might work. @hishamhm what do you think? luarocks is probably the most important application to convince (though luarocks/luarocks#503 already adds luarocks support for lua-zlib.... so perhaps lua-zlib doesn't even need backwards compatibility)

FWIW I did make an abstraction layer of lua-zlib/lzlib: https://github.com/daurnimator/lua-http/blob/master/http/zlib.lua

@hishamhm
Copy link
Contributor

hishamhm commented Oct 3, 2016

Nowadays we also have a compatibility layer in LuaRocks. I think adding a backwards compatible API in lua-zlib is the simplest approach for projects down the road. The lzlib package is under my account in luarocks.org, because I had to tweak Lua 5.3 compatibility into it. Once lua-zlib becomes compatible, I can edit its description to say it's deprecated and direct people to lua-zlib. There are a couple other projects using lzlib in luarocks.org, ideally one should contact them too.

@brimworks
Copy link
Owner

Thanks for the feedback! I'll work on making lua-zlib backwards compatible with lua-lzlib and make sure it works with Lua 5.3. I should be able to do this in the next month.

Thanks,
-Brian

@hishamhm
Copy link
Contributor

hishamhm commented Oct 3, 2016

Awesome! Make sure to ping me when you're done!

@daurnimator
Copy link
Author

btw, you should be able to make the backwards compat layer in pure lua. That should make it easy to maintain :)

@brimworks
Copy link
Owner

Although I like the idea of a pure lua layer, implementing isn't as straight forward (and not as strictly backwards compatible). So, I simply imported the code from lzlib:

7de195f

Feedback is welcomed. I tested this by running luarocks make against lua 5.2 and 5.3. I also manually did some tests using the lzlib interface, but only minimally so.

Thanks,
-Brian

@brimworks
Copy link
Owner

@hishamhm and @daurnimator do you have any comments on my proposal?

7de195f

If I get a thumbs up, then I can upload a new lua rock release and close this issue.

Thanks,
-Brian

@hishamhm
Copy link
Contributor

Sounds good to me! Thank you for the initiative!

@daurnimator
Copy link
Author

Looks good to me.

@brimworks
Copy link
Owner

I've published this as v1.0.0 on luarocks:

https://luarocks.org/modules/brimworks/lua-zlib

Thanks,
-Brian

@daurnimator
Copy link
Author

Thanks! my test matrix is complete for the first time: daurnimator/lua-http@c9fafa1

@daurnimator
Copy link
Author

@hishamhm would you consider pushing people looking at http://luarocks.org/modules/hisham/lzlib over to lua-zlib instead?

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

No branches or pull requests

3 participants