First steps to allowing rock/rockspec URLs as dependencies, incomplete #823
Conversation
Codecov Report
@@ Coverage Diff @@
## master #823 +/- ##
==========================================
- Coverage 86.23% 86.19% -0.04%
==========================================
Files 92 92
Lines 8149 8173 +24
==========================================
+ Hits 7027 7045 +18
- Misses 1122 1128 +6
Continue to review full report at Codecov.
|
|
@mcclure So I finally had some time to give this a closer look. I found the cause of that weird failure, and pushed 7037d5e, which makes it not crash anymore but moves the error to a different place: This is a failure in This opened a bit of a can of worms: all right, so But when we get to We have a few options:
Hopefully this makes sense, feel free to ask any questions! |
Yeah, that's a limitation: the fallback system can only identify new keys, and not existing keys that changed settings. I wonder if it wouldn't be easier/better to just try to typecheck the rockspec with the given format version, and if that fails, try to typecheck again with each later version and any of them succeeds, give an error message like |
I think that would definitely be more user friendly, entirely orthogonal to the URLs issue. I could imagine for example someone getting confused if they try to use namespaces (not allowed in rockspec 1 i think)
I was originally going to suggest we could treat URLs as versions. However, if packages collide across namespaces (ie if NameA/PackageX and NameB/PackageX can't be loaded at once) then I think your suggestion of treating a URL as a namespace is actually perfect. If I'm understanding the current regime correctly namespaces are more like an "origin" tag than anything else, which is also how the URLs are used. And they even use slashes. I think we should go with solution 3ii. If this is the approach, maybe should I move the URL parsing to occur earlier (IE, having the URL parsed in the query object?) Which of us should write the patch for the next step? |
That is true (great point about namespaces!). I'll take care of this.
Yes, "origin tag" is a great description — I'll quote you on that next time I have to explain LuaRocks namespaces to people! (I thought of several possible designs for adding namespaces to LR, with various degrees of complexity and compatibility issues, which ended up holding me back from implementing for a long time; during a meetup people convinced me to just go with the simplest solution — this one strikes a balance between providing separation for those who use namespaces explicitly and still playing ok with non-namespaced usages because sometimes people do install
3ii is my favorite as well! Yes, URL parsed in the query object makes the most sense. Then you can special-case behavior based on the presence of the
I'll take care of the typechecker — could you do the URL parsing and further handling of |
|
I'll rebase on 7037d5e, take a look and see if I can push another commit. Thanks! By the way, the new approach is going to make things a little more confusing when/if we try to add git support. |
Yeah, I've been thinking about that. Now what would a That doesn't point to a rockspec very clearly. The |
|
(git support is more forward-thinking stuff, I think — I'd limit the scope of this PR to file:// and http(s):// only — after all today you can do |
|
I think a git dependency would need 3 pieces of information included: a git repo, a git revision, and a path to a rockspec file. I assume we'd have to separate them by spaces. I found one open source tool that uses // to indicate a file in a repo, ie, domain.com/gitrepo.git//path/to/file.txt , but that is hardly standard. I definitely think it makes more sense to get the file:// https:// change in first and then worry about git afterward. Of course for github there's always blob urls… |
|
New commit allows use of http and https dependencies in my tests. Next step, file:// , then I'll try to do some tests. |
|
Oh, I didn't populate the namespace field of "query". I left it nil. Is there logic that that is inappropriate for? |
Codecov Report
@@ Coverage Diff @@
## master #823 +/- ##
=========================================
Coverage ? 86.75%
=========================================
Files ? 90
Lines ? 8885
Branches ? 0
=========================================
Hits ? 7708
Misses ? 1177
Partials ? 0
Continue to review full report at Codecov.
|
|
A couple more things.
One for each file in cpml. Do you know what this might mean?
At the moment, file:// URLs may begin with . but the path taken is relative to the pwd which luarocks was executed from. So the change here would be to change the pwd interpreted in deps.fulfill_dependency to be the directory the including dep is found in. The "./" approach might be better though because it would work for non-local rockspecs (for example, rock A has dependency https://blah/BLAH.rockspec and BLAH has dependency ./OTHER.rockspec). What do you think, is this feature worth bothering with? |
It won't crash (nil means "no namespace"), but since in solution 3ii we want to store the URL as a namespace for the rock, you can store
Odd. This means that those files are not being properly stored in the manifest. Please take a look at Also, if you delete your
That would generally work for
...which could be tricky (or easy to implement), I don't know. Of the two approaches, I like
It does sound like an edge case. I'd welcome it if it doesn't add a lot of complexity. |
I tried adding this (setting the namespace to the URL-minus-filename, as you suggested). It resulted in this failure The failure occurs because in match_dep, "versions" is an empty array for the URL query and match_dep returns nil. Do you know why? Also purely because I'm curious, what code is supposed to be reading that namespace back out, once it is placed in the query object, since search.find_suitable_rock is now bypassing it?
I think it would be easy and I think I know how to do it, but let's consider it out of scope for this PR. I'll make a new PR and see if I can make it make sense |
|
Also would it be possible to get some assistance with creating a test in the spec/ folder? It seems the tests are done in a particular way. All we need for the test is to have a local rockspec that has the https address of the github blob for one of the rockspecs used in the other tests, then add a test in spec/make_spec.lua to make sure the dependency test is installed. I think a file:// test is not possible without relative paths. Here's the file I've been using for local tests: pseudopseudocpml-a-1.rockspec.txt I think this PR can be possibly merged once the namespace and "is not tracked by this installation of LuaRocks" issues are resolved, and once we have a test in spec/? Is there anything else to be done? |
* luarocks/type_check.lua:
* fix expansion of platforms
* luarocks/type/rockspec.lua:
* when a version fails checking, try with a later
version to produce a hint in the error message.
See #823.
* luarocks/type_check.lua:
* fix expansion of platforms
* luarocks/type/rockspec.lua:
* when a version fails checking, try with a later
version to produce a hint in the error message.
See #823.
* luarocks/type_check.lua:
* fix expansion of platforms
* luarocks/type/rockspec.lua:
* when a version fails checking, try with a later
version to produce a hint in the error message.
See #823.
Not sure. I haven't run your branch locally to test yet, but I wrote a quick test here to check that URLs and the --namespace flag play nicely with each other and that passed in current diff --git a/spec/install_spec.lua b/spec/install_spec.lua
index 8a3ede3..0975a58 100644
--- a/spec/install_spec.lua
+++ b/spec/install_spec.lua
@@ -98,6 +98,14 @@ describe("luarocks install #integration", function()
assert(lfs.attributes(testing_paths.testing_sys_rocks .. "/a_rock/2.0-1/rock_namespace"))
end)
+ it("installs a namespaced package given an URL and any string in --namespace", function()
+ assert(run.luarocks_bool("install --namespace=x.y@z file://" .. testing_paths.fixtures_dir .. "/a_rock-1.0-1.src.rock" ))
+ assert.truthy(run.luarocks_bool("show a_rock 1.0"))
+ local fd = assert(io.open(testing_paths.testing_sys_rocks .. "/a_rock/1.0-1/rock_namespace", "r"))
+ finally(function() fd:close() end)
+ assert.same("x.y@z", fd:read("*l"))
+ end)
+
it("installs a package with a namespaced dependency", function()
assert(run.luarocks_bool("install has_namespaced_dep --server=" .. testing_paths.fixtures_dir .. "/a_repo" ))
assert(run.luarocks_bool("show has_namespaced_dep"))I just merged a big branch — including the rockspec format version checking changes we discussed, which is the cause of the current merge conflict — so not sure if that affected the above problem some way. When you get your branch in sync I'll fetch and run it locally to see if I can spot anything related to the namespace issue. |
Yeah, the test suite is very, erm, peculiar, as it has a long history behind it (started as a big shell script, ported to Lua — it's currently being improved a lot by @georgeroman as his GSoC project!)
We're now trying to write new tests so that they don't rely on live internet URLs. We do launch a tiny HTTP server in the test suite so This is an example of a test using it: https://github.com/luarocks/luarocks/blob/master/spec/build_spec.lua#L437-L442 It uses this file: https://github.com/luarocks/luarocks/blob/master/spec/fixtures/with_external_dep-0.1-1.rockspec Tests using the HTTP server should be inside a https://github.com/luarocks/luarocks/blob/master/spec/build_spec.lua#L394-L402
We can write a rockspec on the fly, like this: https://github.com/luarocks/luarocks/blob/master/spec/build_spec.lua#L299-L322 |
|
Rebased the commits and fixed the merge conflicts. Still seeing the failure (but only when I include the commit which handles namespaces correctly). Here is my test case w/instructions Here is the error I get The "assert" is that match_dep returns nil Can you take a look at this? |
Supports http:// and https:// urls. Todo add support for file://, add tests
Here's my first pass. I did a version based on 06a4656, which worked but printed a spurious error in report_missing_dependencies. This new version based against master (I changed the _pattern format, as we discussed) but it does not get as far. It appears to have a similar problem to my 06a4656-based version but more catastrophic. The error is
The problem is match_dep expects the argument, which is pulled out of the dependency table, to be a table and instead it is a string (the http url). This is linked only to the pattern change in rockspec.lua— the error occurs before find_suitable_rock is called, and occurs even if the find_suitable_rock changes are commented out. I'm not sure how this string is getting put in the dependency table. Based on my experience with 06a4656 I think if this is fixed the patch will work.
I can provide my testing files if it would help.
Other issues:
TODOS: