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

Improve error on archive unpack error #574

Merged
merged 2 commits into from
Jun 22, 2016

Conversation

mpeterv
Copy link
Contributor

@mpeterv mpeterv commented Jun 22, 2016

Currently fetch ignores archive unpack errors and simply checks if expected source directory exists. This may be a workaround to support an undocumented feature that allows to use an url pointing directly to a .lua or .c file, but fs.unpack_archive already ignores such files.

With this PR unpack errors are detected earlier, and use a different message for unrecognized archive extension, which is a common error that happens when wrongly using https:// in place of git+https://:

Directory <name> not found inside archive <name>.git

becomes

Couldn't extract archive <name>.git: unrecognized extension.

@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 22, 2016

Would be nice if luarocks could also say something like The rockspec seems be using https:// protocol instead of git+https:// or hg+https:// in this common case, not sure where to put that. Or maybe we could just improve the rockspec creation guide focusing more on scm-based ones.

@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 22, 2016

The tests fail because find_base_dir can be called on urls using non-basic protocols, and unpack fails on them.

In particular, when installing a rock, fail with a message related
to extraction error or unrecognized archive extension instead of
'Directory <name> not found inside archive <name>.<ext>'.
@@ -192,8 +192,7 @@ function tools.unpack_archive(archive)
-- Ignore .lua and .c files; they don't need to be extracted.
return true
else
local ext = archive:match(".*(%..*)")
return false, "Unrecognized filename extension "..(ext or "")
return false, "Couldn't extract archive "..archive..": unrecognized extension"
Copy link
Member

Choose a reason for hiding this comment

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

I think it should keep saying "filename extension"; "extension" is vague.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@hishamhm
Copy link
Member

Or maybe we could just improve the rockspec creation guide focusing more on scm-based ones.

But the guide already points to git:// first, no?
https://github.com/keplerproject/luarocks/wiki/Creating-a-rock

The right thing to do, in luarocks-3, would be to detect .git URLs. Do any other SCMs use filename extension like that to identify an SCM URL or is this just a Git thing? The fetch system could be adjusted to try detection by filename extension first, then protocol.

@hishamhm
Copy link
Member

Also, which tests fail?

@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 22, 2016

Or maybe we could just improve the rockspec creation guide focusing more on scm-based ones.

But the guide already points to git:// first, no?
https://github.com/keplerproject/luarocks/wiki/Creating-a-rock

Right, I looked at rockspec format page, it could pronounce it a bit better that fields such as source.dir are irrelevant for scm urls. It's really just nitpicking though.

The right thing to do, in luarocks-3, would be to detect .git URLs. Do any other SCMs use filename extension like that to identify an SCM URL or is this just a Git thing? The fetch system could be adjusted to try detection by filename extension first, then protocol.

But .git extension is optional, so it shouldn't be possible to detect all git urls.

Also, which tests fail?

new-version and write-rockspec tests that run corresponding commands on scm rockspecs, I've fixed it.

Return `Couldn't extract archive <file>: unrecognized filename extension`
instead of `Unrecognized extension <ext>`, so that it's clear
that the file is being interpreted as an archive.
@hishamhm
Copy link
Member

But .git extension is optional, so it shouldn't be possible to detect all git urls.

Sure, a plain "https://github.com/user/project" can't be detected, but I notice that users assume that the .git extension is a way of telling LuaRocks that the URL is a Git repo.

@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 22, 2016

@hishamhm that could work, but what would the standard way of doing things be, 'git+https://foo.com/bar' or 'https://foo.com/bar.git'? Or do you want to remove the first one?

@hishamhm
Copy link
Member

I'd keep them both. I guess Git users would naturally grativate towards the latter (it's the default URL Github gives you for copy-pasting), but we'd still have to handle foo:// protocols for the other SCMs.

@mpeterv
Copy link
Contributor Author

mpeterv commented Jun 22, 2016

Ok, that could be helpful. We'll have two ways for doing one thing, but that's fine as one of them is more general than the other. Sorry for derailing this with my suggestions, can this PR be merged?

@hishamhm hishamhm merged commit fa60a09 into luarocks:master Jun 22, 2016
@hishamhm
Copy link
Member

Sure!

@mpeterv mpeterv deleted the unpack-archive-err branch June 22, 2016 19:37
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

Successfully merging this pull request may close these issues.

None yet

2 participants