#197 breaks coro-http - Githubissues

SinisterRectus commented 7 years ago

Fresh installation:

sinister@ubuntu:~/new$ ./lit install creationix/coro-http
lit version: 3.5.2
luvi version: v2.7.6
command: install creationix/coro-http
load config: /home/sinister/.litconfig
connecting: wss://lit.luvit.io/
fetching: 1 object
fetching: 1 object
fetching: 1 object
fetching: 1 object
fetching: 1 object
including dependency: coro-channel (3.0.0)
including dependency: coro-http (2.2.0)
including dependency: coro-net (3.0.0)
including dependency: coro-wrapper (3.0.0)
including dependency: http-codec (3.0.1)
installing package: creationix/coro-wrapper@v3.0.0
installing package: creationix/coro-channel@v3.0.0
installing package: creationix/coro-http@v2.2.0
installing package: creationix/coro-net@v3.0.0
installing package: luvit/http-codec@v3.0.1
done: success

Reduced test code:

local http = require('coro-http')

coroutine.wrap(function()
    local res, data = http.request('GET', 'http://google.com/')
    p(res)
    p(data)
end)()

Error:

sinister@ubuntu:~/new$ ./luvit test.lua
Uncaught Error: /home/sinister/new/deps/coro-channel.lua:62: /home/sinister/new/deps/coro-wrapper.lua:109: index must be a number if set
stack traceback:
        [C]: in function 'assert'
        /home/sinister/new/deps/coro-channel.lua:62: in function </home/sinister/new/deps/coro-channel.lua:54>
        [C]: in function 'run'
        [string "bundle:init.lua"]:52: in function <[string "bundle:init.lua"]:47>
        [C]: in function 'xpcall'
        [string "bundle:init.lua"]:47: in function 'fn'
        [string "bundle:deps/require.lua"]:309: in function <[string "bundle:deps/require.lua"]:265>

squeek502 commented 7 years ago

As an aside, it'd also be nice to get some tests written for the coro- packages.

creationix commented 7 years ago

Yes, some dedicated tests and docs for the coro-* libs would be awesome.

I was testing this against the lit unit tests which I thought would be sufficient. I was wrong.

squeek502 commented 7 years ago

Found the issue: http-codec from the Luvit bundle (http-codec@2.0.0) is taking priority over the local deps/http-codec (http-codec@3.0.1).

To show this, here's the output running the test script with luvit:

C:\coro-http-test>luvit test.lua
Uncaught Error: [string "C:/coro-http-test/deps/coro-channel.lua"]:62: [string "C:/coro-http-test/deps/coro-wrapper.lua"]:109: index must be a number if set

And here's the output running the test script with luver:

C:\coro-http-test>luver test.lua
table: 0x1b64b8d8       <HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="http://www.google.com/">here</A>.
</BODY></HTML>

creationix commented 7 years ago

Oh, I do remember seeing this, but that was before I got sick and checked out.

I guess I need to update luvit itself to use the new version or change the require resolution to not put internal versions of libraries ahead of disk versions. It's not a safe change since both ways carry dangers.

This is all the more reason to make luvit 3.0 that has a nearly empty standard library. Lit makes dependency management easy enough we don't need much built-in.

creationix commented 7 years ago

Just going to link to this again for people who aren't aware. This is the direction I mean for luvit 3.0 https://github.com/squeek502/luver. I remember we had some issues with the luvit-loader code and relative path resolution interacting bad with lua module caches.

squeek502 commented 7 years ago

Yeah, here's the PR for Luvit with that discussion: https://github.com/luvit/luvit/pull/932. We never really came to a conclusion on what to do about relative requires (and it's something I'll need to deal with in luver, too).

SinisterRectus commented 7 years ago

I've made a legacy branch that has the original coro libraries. Is there any way to format lit package metadata so that the dependencies are pulled from here (a specific GitHub branch)?

squeek502 commented 7 years ago

You could alter the metadata of the relevant files to publish the older libraries under your own author name and then include those in your package.lua (e.g. SinisterRectus/coro-net@2.2.0).

See https://github.com/luvit/lit/wiki/Creating-Your-First-Package#single-file-package

SinisterRectus commented 7 years ago

Of course. I just didn't want to clutter lit if there is a better way to host temporary libraries.

squeek502 commented 7 years ago

Not that I'm aware of; calculate-deps expects author/name formatting exclusively.

creationix commented 7 years ago

You should be able to continue using the coro-* 2.x libraries. Just tell your app it depends on creationix/coro-net@2? Or is the problem transitive dependencies on things like coro-http that didn't have a major bump?

SinisterRectus commented 7 years ago

coro-http 2.2.0 is the culprit, and the previous version (2.1.1) is not accessible because lit defaults to the most recent, related "non-breaking" version, which is 2.2.0.

SinisterRectus commented 7 years ago

As a workaround, I've re-published coro-http@2.1.1 and coro-websocket@1.0.0-1 to lit under my name, with no other changes.

creationix commented 7 years ago

I guess I should have made the coro-http release 3.x as well. I can do that if you'd like?

creationix commented 7 years ago

Ok, republished 2.1.1 as 2.3.0 and 2.2.0 as 3.0.0. Let me know if this lets you use 2.x again.

And sorry for the slow responses, my kids have been sick lately (the flu is going around).

SinisterRectus commented 7 years ago

Yes, that allows coro-http 2.x to be installed. Thanks. I realized that coro-websocket has the same issue, though. I'm using my re-published dependencies for now, but you'll probably want to tweak the coro-websocket version, too. I haven't tested any other libraries that might be affected (ie, the weblit ones).

creationix commented 7 years ago

Thanks again for reporting this and debugging the issues.

creationix commented 7 years ago

Ok, did the same thing for coro-websocket so that the 1.x series can still be used with luvit 2.x.

creationix commented 7 years ago

Yeah, weblit is broken too, but that's a harder fix. I'll think on that one for a while.

Personally I would like to move people away from luvit to just luvi + lit.

I did just create an alpine based docker image for easy deployment of luvi based apps to any docker service.

Here is a sample application that's luvi and weblit based using the new 3.x coro-* libs. https://hub.docker.com/r/creationix/hexes/

The new creationix/lit:alpine docker image makes this really easy https://github.com/creationix/hexes/blob/master/Dockerfile

SinisterRectus commented 7 years ago

I'm all for moving away from luvit 2.0 and towards something more like luver or a stand-alone luvi app with coroutine libraries. Perhaps for my project's next major version bump, or if luvit 3.0 becomes a reality.

creationix commented 7 years ago

The main issue with a luvit 3.0 is the issues with luvit-loader that are pretty much unsolvable. Those issues have never bitten me personally, but I'd hate to move to a new require system knowing there are dragons hidden in the corners.

Telling people to switch to luvi punts on this problem by making luvit-loader the defacto standard, but technically user-space and not part of core.

SinisterRectus commented 7 years ago

This was fixed by various commits starting with luvit/luvit@c8da7e72d80e7ae0df92839f54feddd0252facc9. We just need a formal luvit release.

creationix commented 7 years ago

Formal release incoming https://github.com/luvit/luvit/pull/980

luvit / lit

#197 breaks coro-http #200