Skip to content

Conversation

@maxguru
Copy link

@maxguru maxguru commented Jun 14, 2016

No description provided.

endef

define Build/Configure
endef
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need this block.

Copy link
Author

Choose a reason for hiding this comment

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

Can you please clarify? Why? Which block exactly? I'm pretty sure define Package/cgilua is needed. Maybe define Build/Configure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the configure blocks. (in all these packages)

Copy link
Author

Choose a reason for hiding this comment

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

Please take a look at my fix.

@diizzyy
Copy link
Contributor

diizzyy commented Jan 7, 2017

@karlp
Can you please have a look at this?

@@ -0,0 +1,61 @@
#
# Copyright (C) 2016 OpenWrt.org
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct. You aren't OpenWrt. Either drop teh copyright outright (my choice) or provide your own name here. (will apply in all places)

Copy link
Author

Choose a reason for hiding this comment

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

I was under the impression that was required. I'll fix it.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, according to this I can use OpenWrt.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's incorrect unfortunately. Either drop it as karlp suggests or add your own.

PKG_VERSION:=5.1.4
PKG_RELEASE:=1

PKG_SOURCE:=v$(PKG_VERSION).tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a no go, use the git approach instead including xz tarballs.

Copy link
Author

Choose a reason for hiding this comment

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

Why not? What rule does this break?

Copy link
Contributor

@diizzyy diizzyy Jan 10, 2017

Choose a reason for hiding this comment

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

You'll run into file names clashing if they having nothing more than v(version).tar.gz as file name.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right.

define Package/cgilua/install
$(INSTALL_DIR) $(1)/$(LUA_MODULE_PATH)/cgilua
$(INSTALL_DATA) $(PKG_BUILD_DIR)/src/cgilua/cgilua.lua $(1)/$(LUA_MODULE_PATH)/cgilua.lua
$(INSTALL_DATA) $(PKG_BUILD_DIR)/src/cgilua/authentication.lua $(1)/$(LUA_MODULE_PATH)/cgilua/authentication.lua
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? the upstream package keeps them in the same dir and needs them in different dirs? Does the upstream package have a makefile with an "install" target you can use?

Copy link
Author

Choose a reason for hiding this comment

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

I'll have to test to make sure the other way works.


PKG_NAME:=jsonrpc4lua
PKG_VERSION:=1.0.1
PKG_RELEASE:=2
Copy link
Contributor

Choose a reason for hiding this comment

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

no. pkg_release is obviously 1 here.

@@ -0,0 +1,49 @@
#
# Copyright (C) 2006-2015 OpenWrt.org
Copy link
Contributor

Choose a reason for hiding this comment

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

definitely not 2006! (or 2015!)

endef

define Package/jsonrpc4lua/description
JSONRPC for Lua
Copy link
Contributor

Choose a reason for hiding this comment

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

Great description! Is this for clients? for servers? for generating stubs only? Who knows!

LUA_MODULE_PATH:=/usr/lib/lua

define Build/Compile
$(TARGET_CROSS)gcc $(TARGET_CFLAGS) $(TARGET_LDFLAGS) -I $(PKG_BUILD_DIR)/c-api/ -o $(PKG_BUILD_DIR)/bit32.so $(PKG_BUILD_DIR)/lbitlib.c
Copy link
Contributor

Choose a reason for hiding this comment

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

Is upstream really that broken that you have to compile it yourself? Have you considered just using lua-bitop? it's already packaged.

Copy link
Author

Choose a reason for hiding this comment

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

If I recall, at the time bit32 looked better and more future proof. I didn't realize there were other ways to compile it. I'll have to check if I can rework it.

PKG_SOURCE_VERSION:=master

PKG_MAINTAINER:=Alex R <alex@totalwebservices.net>
PKG_LICENSE=Simplified BSD license
Copy link
Contributor

Choose a reason for hiding this comment

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

https://spdx.org/licenses/ I think you want "BSD-2-Clause" here


PKG_NAME:=luaproc
PKG_VERSION:=1.0
PKG_RELEASE:=4
Copy link
Contributor

Choose a reason for hiding this comment

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

no, should be 1

@karlp
Copy link
Contributor

karlp commented Jan 9, 2017

You've used different names and emails for some of these packages, was that intentional? Are you really sure that none of the existing packages met your needs for any of these?

@maxguru
Copy link
Author

maxguru commented Jan 10, 2017

Regarding different names/emails, I was still in the process of separating identities.

Regarding possible duplication of functionality, I don't believe that's a valid concern.

jsonrpc4lua: The original json4lua package was only semi-functional. The JSON-RPC server component didn't work at all. Initially, I was trying to fix it (see craigmj/json4lua#13) but then, I realized it was trying to do too much. JSON-RPC features should not be bundled into JSON encoding/decoding library. The new fork uses lua-cjson for encoding/decoding and has actually functioning JSON-RPC client and server code.

bit32: I was surprised this wasn't already in the repository, given the version of Lua that was used. "bit32 is the native Lua 5.2 bit manipulation library, backported to Lua 5.1"

alt-getopt: I found this package useful, but it can be left out.
inifile: I found this package useful, but it can be left out.

cgilua: I was also surprised that the repository is missing this package, given that it has lua-wsapi and lua-xavante. Also, I believe this is a dependency for the server component of jsonrpc4lua (and the non-functioning JSON-RPC server code in json4lua).

@maxguru
Copy link
Author

maxguru commented Jan 10, 2017

I forgot to talk about luaproc. It is an excellent library. Very good alternative to Lua Lanes.

@karlp
Copy link
Contributor

karlp commented Jan 10, 2017

You'll want to squish the fixup commits at some point too btw.

Copy link
Member

@champtar champtar left a comment

Choose a reason for hiding this comment

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

Hi @maxguru
you are missing signed off by on all you commits

@maxguru
Copy link
Author

maxguru commented Jul 18, 2017

Rant:

Here is the problem. I was working on the project that triggered the commits prior to June of 2016. I created the pull request because I thought my packages were useful. Nobody responded to my pull request back then. Then after all these months somebody decided to respond finally. I already moved on and no longer have the environment set up. Once I get back to the project, I will see about fixing the couple of issues that I still have left to fix. If somebody else is up for it before then, feel free to do it.

@champtar, I wasn't aware that I needed to sign off commits. I would need to recreate the pull request to fix this, no? I might as well create individual pull requests for each package if that is the case.

@champtar champtar closed this Aug 18, 2017
@champtar champtar reopened this Aug 18, 2017
@diizzyy
Copy link
Contributor

diizzyy commented Dec 30, 2018

@karlp @thess @hnyman
Close due to submitter timeout?

@maxguru
Copy link
Author

maxguru commented Dec 30, 2018

Unfortunately, I haven't had the time to do what was requested. If somebody wants to take this on, that would be great.

@thess
Copy link
Member

thess commented Dec 31, 2018

Way to old - closing.

@thess thess closed this Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants