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

LuaRocks 3.x #7

Merged
merged 9 commits into from
Jun 16, 2019
Merged

LuaRocks 3.x #7

merged 9 commits into from
Jun 16, 2019

Conversation

zwirec
Copy link

@zwirec zwirec commented Jun 13, 2019

No description provided.

* Skip external commands loading if no external_namespace provided: it
  is so when a command is run using `tarantoolctl rocks <command>`.

* Allow to omit a description field in a rockspec.

* Configure more options using hardcoded.lua (it is named as
  site_config.lua in luarocks-2.x).

  Based on or inspired by the following commits:

  - 9f55766 ('Add LUAROCKS_LOCAL_BY_DEFAULT option to site_config.lua').
  - e3f7486 ('Allow to override certain values in cfg.lua').
  - dba49e0 ('Enable cfg.home_tree even for root').
  - 6e6fe62 ('Add site_config.LUAROCKS_LOCALDIR config variable').

  The following options are added for hardcoded.lua:

  - LOCALDIR
  - LOCAL_BY_DEFAULT
  - HOMEDIR
  - HOME_TREE_SUBDIR
  - LUA_MODULES_LUA_SUBDIR
  - LUA_MODULES_LIB_SUBDIR
  - ROCKS_SERVERS

  The following options are ported from luarocks-2 (and renamed from
  LUAROCKS_FOO to FOO):

  - ROCKS_SUBDIR
  - EXTERNAL_DEPS_SUBDIRS

  All that options are needed to adjust behaviour of luarocks from
  tarantool-side hardcoded.lua file.

* Detect tarantool and add it to rocks_provided and rocks_provided_3_0.

  Based on:

  - 370d173 ('Detect and add Tarantool it to rocks_provided)'

* Execute cfg.init() code only once.

* Fixed strict mode.

  Inspired by:

  - dfaf765 ('Fix strict mode').

* Dropped site_config.lua support: tarantool doesn't use it anymore.
@Totktonada
Copy link
Member

Usually we comment a patchset, but here the deadline is near, so I decided to
make changes myself. I have pushed the original branch to luarocks-3.x-orig
branch, so you can verify the changes.

I reverted some of changes, because failed to understood what is a problem and
how to check that it is resolved. I hope you'll answer here whether they are
actually needed and why; and we'll proceed with them (within this PR or
another ones depending on timing). See details below.

Below I have summarized changes I made for the patchset, its motivation and
checks I performed during the review.

Re first commit ('Fix compatibility with tarantool static build'):

  • Squashed two commits re compatibility with tarantool: 'Fix compatibility
    with tarantool static build' and 'patch defaults for using with tarantool'.
  • Updated the description of the commit re compatibility with tarantool:
    described all made changes, mention relevant commits from tarantool-1.7
    branch, listed all hardcoded.lua options that was added or ported from
    luarocks-2.
  • Removed localdir = require('fio').cwd() in env_for_config_file(),
    because in set afterwards in set_confdirs() using hardcoded.LOCALDIR
    value. (@rosik suggested me that it possibly set here to be seen in
    hardcoded.lua, but it is not used in tarantool's hardcoded.lua file.)
  • Fixed indentation in the 'rocks_provided' code block.
  • Renamed cfg.inited to cfg.initialized. BTW, don't get why it is needed, but
    don't want to dig into that.
  • Removed 'we don't need to workaround older luarocks' comment from the code
    (because it actually deletes the code which comments and becomes dangling),
    commented it in the commit message instead.

Re commit 'pass destination dir to unpack rocks archive':

  • I don't get why this is needed in the first place. Nothing in the commit
    message helps to understand it (initially my thought was that it adds an
    option to specify a directory to unpack a rock to).
  • source_dir is passed to unpack_archive() in elseif branch in
    fetch_and_change_to_source_dir(), but here it is always nil. Looks as
    the mistake.
  • The patch changes several calls to unpack_archive(). I tried only one case
    (described in the bullet below). The commit message does not give me any
    pointers what was changed aside of the case below. So to review the patch I
    need to dig into the code and elaborate all possible cases where a behaviour
    can be changed by the patch. I think it is the responsibility of a developer
    to describe what is the behaviour that was changed, how exactly it was
    changed and, more importantly, why.
  • I have cloned repo of avro-schema and packed it with luarocks pack avro-schema-scm-1.rockspec to at least see what the patch doing. Before the
    patch tarantoolctl rocks unpack /path/to/avro-schema-scm-1.src.rock
    produces avro-schema-scm-1/avro-schema directory with the repository
    content (including the rockspec). After the patch it produces the same
    avro-schema-scm-1/avro-schema directory, but it contains only two files:
    avro-schema directory with the repository content (including the rockspec)
    and the rockspec (again).
  • I see that it is some layout change, but I don't know why this is needed and
    cannot review it. Removed the patch from the patchset.

Re commit 'wrap call other loaders in pcall':

  • Again, nothing says why the change is needed.
  • A call of a loader was wrapped into a table to handle multireturn. After the
    change only one return value will be returned (a function). While it seems
    that a loader normally returns only one value (a function, a string or nil)
    and is called with lua_call(L, 1, 1) in lj_cf_package_require(), there is no
    reason to discard multireturn here: maybe luarocks itself or, say, busted
    uses expanded convention here (I don't know).
  • The contract of a loader obligates it to return nil or a string in case
    of an error. It should not raise an error.
  • So I need to know what is the exact problem the commit solves. Removed the
    patch from the patchset for now.

Re commit 'stop scanning rocks servers if exact match found':

  • Removed trailings spaces removing and empty line changes. Yep, it is bad
    when you leave it in your code, but it is not enough reason to change it
    when it is not a part of your changes. This can make reading your patch,
    blaming changes in the future and backporting patches from an upstream more
    complex. The general rule here is to avoid changes that are not related to
    your patch.
  • Removed forgotten commented print (BTW, I already noticed it in here).
  • I see the difference with the original Yaroslav's patches ('Stop scanning
    rocks servers if exact match found' and 'Optimize repos scanning'): you call
    search.local_manifest_search() if a file URI is passed as a repository
    URI. Is it the fix for some luarocks regression (if so it should be
    mentioned in the commit message)? I put a rock file for avro-schema into a
    directory, created a repo with luarocks-admin make_manifest . and added it
    as the first repository into hardcoded.lua. Then disabled a network and
    tried with search.local_manifest_search() and with
    remote_manifest_search(). In both cases tarantoolctl rocks install avro-schema scm-1 does not report a network error. So I removed this change
    from the patch.
  • Splitted the long condition in if as it was made in the 2nd Yaroslav's
    patch.
  • Added a link to the problem description to the commit message.
  • Mentioned the original commits and the author in the commit message.
  • Titlecased the commit header.

I'll continue tomorrow. The plan is the following:

  • Review remaining patches in the patchset.
  • Verify that patches that are not backported from tarantool-1.7 branch are
    not needed anymore (in other words that moving to luarocks-3.1.1-tarantool
    branch will not lead to a regression).
  • Run some commands to ensure that at least basic commands works as expected.
  • Verify new commands: at least tarantoolctl rocks test.

Konstantin Belyavskiy and others added 8 commits June 16, 2019 13:14
A module written on C requires tarantool header files. Usually a
rockspec checks for its existence and pass a directory to use inside a
build system (say, cmake).

The check for existence may be performed like so:

 | external_dependencies = {
 |     TARANTOOL = {
 |         header = 'tarantool/module.h'
 |     }
 | }

While the directory may be passed in this way:

 | build = {
 |     type = 'cmake',
 |     variables = {
 |         TARANTOOL_DIR='$(TARANTOOL_DIR)',
 |     }
 | }

Consider 'ckit' branch of 'tarantool/modulekit' repository for a full
example.

The check for external dependencies fails when tarantool is installed
outside of '/usr/ or '/usr/local/' directories. Consider [1] for more
information about the problem.

To fix the problem hardened.PREFIX, which is filled during tarantool
build with CMAKE_INSTALL_PREFIX value, is added to the default value of
'external_deps_dirs' list of directories. Luarocks use this value to
resolve external dependencies.

The patch is based on the following patch for luarocks-2:

- b2dde9a ('[luarocks] Add support for non-standard path')

[1]: tarantool/tarantool#3148
The problem is described in #3

Based on the following patches:

- f8813b4 ('Stop scanning rocks servers if exact match found')
- 2f49670 ('Optimize repos scanning')
luarocks has support for calculating md5 with 'md5' rock if it's
present, but we don't have it in tarantool, and instead have the
'digest' module. That's why luarocks falls back to 'openssl' binary to
calculate md5 digests.

This patch will allow luarocks to use our internal digests module.

(cherry-picked from 92b5f7d)
Tarantool has no separate dynamic library (all necessary symbols are
exposed from its executable file), so we should skip the check for a Lua
dynamic library existence.

Tarantool's headers (such as lua.h) are placed in
${PREFIX}/include/tarantool, but LUA_INCDIR is always set using
hardcoded.lua, so we don't need to adjust find_lua_incdir() code. This
function tries to guess LUA_INCDIR based on LUA_DIR.
Luarocks tries to detect a Lua interpreter version installed in a
system. When this attempt fails it gives a warning that built or
installed modules may be configured improperly (say, configured for 5.1,
but intended to be run with 5.3).

If there are no --lua-ver[sion] and --lua-dir command-line options or
corresponding configuration values luarocks still tries to find a Lua
interpreter, but fails to give a warning due to concatenation a string
with `nil`. The error looks so:

> /usr/share/tarantool/luarocks/cmd.lua:392: attempt to concatenate
> local 'lua_version' (a nil value)'

The patch changes a warning to report a Lua version from a configuration
when the command line options are not passed.

See luarocks#961
This change eliminates the following warning when tarantool is the only
Lua interpreter installed on the system:

> Warning: Could not find a Lua interpreter for version 5.1 in your
> PATH. Modules may not install with the correct configurations. You may
> want to specify to the path prefix to your build of Lua 5.1 using
> --lua-dir
@Totktonada
Copy link
Member

Updated the PR again.

Re commit 'Stop scanning rocks servers if exact match found' (again):

  • Set the original author: there are no significant changes; removed author
    mention from the commit message.

Re commit 'add support for non-standard path':

  • Set the original author for the commit.
  • Clarified the problem in the commit message.
  • Linked the issue with the problem description.
  • Mentioned the original patch in the commit message.
  • Titlecased the commit header.
  • Moved before 'Stop scanning rocks servers if exact match found', because
    that was the original order in tarantool-1.7 branch.

Re commit 'Use digest.md5_hex to compute md5 digests instead of openssl':

  • Moved right after 'Stop scanning rocks servers if exact match found' as in
    the original patchset (tarantool-1.7).
  • Set the original commit author.
  • Added 'cherry-picked from' clause.

Re commit 'don't check lua when process dependencies':

  • Rewritten it in less intrusive way, see the commit 'Skip Lua dynamic library
    search for tarantool'. It only skips the check for a Lua library existence
    when tarantool is set as the Lua interpreter.
  • Added proper motivation into the commit message.

Re commit 'test: fix reporting failures on 'command' backend':

  • Cherry-picked the version of the patch that was land to the upstream
    (instead of intermediate version).

Re commit 'fix check default lua_version':

  • nil or smth will always give smth.
  • It was to give a warnings that Yaroslav removed later in 'Get rid of
    useless warning' commit instead of raising the error re concatenation with
    nil (cited below). I performed many tests to understand why your patch is
    needed and only found it by occasion when experimenting around the
    Yaroslav's patch.
  • It is trackerized in attempt to concatenate a nil value luarocks/luarocks#961 . Worth
    to mention.
  • As I see from the commit where get_lua_version() was introduced (a21fcdc)
    the value it returns is about --lua-ver[sion] and --lua-dir command-line
    options and a saved configuration value (inside an installed rock?). It is
    intended to be nil when unset. Different Lua interpreter detection logic
    works depending on whether it is nil or not.
  • So I doubt this is the right way to fix the issue. I removed the patch from
    the patchset, but fixed the warning to report cfg.lua_version when the
    options mentioned above are not specified (commit 'Fix concatenate with a
    nil value when give a warn').

/usr/share/tarantool/luarocks/cmd.lua:392: attempt to concatenate local
'lua_version' (a nil value)'

Re commit 'fix strange string.match incorrect behavior':

  • Again, there is no description why this change it needed. I already
    mentioned that in the PR to tarantool re luarocks-3).
  • This function is called from two places and the set of values for which
    :match() is called is known: 'string', 'string?', 'boolean', 'boolean?'. You
    changed '^(.-)(%??)$' to '^(.-)(%?-)$', but they are equivalent when only
    zero or one question mark is at end of a string. I verified this with
    tarantool.
  • It seems this patch actually does not change any behaviour. I removed it
    from the patchset.

Re commit 'Get rid of useless warning':

  • We can support searching for tarantool as the Lua interpreter instead for
    silence the warning. I think this is good step toward support tarantool in
    the upstream luarocks. Aside of that this way looks more clean for me. I
    made the patch 'Detect tarantool as the Lua interpreter'.
  • Now the warnings means for us that hardcoded.lua contains broken paths, so I
    removed your patch about removing this warning from the patchset.

I verified tarantool/tarantool#2609 , found the
regression and cherry-picked 'Add --recursive to git clone' commit from
tarantool-1.7.

I think that less intrusive and properly described changes allow us to
upstream most of them. It can help us to reduce maintenance costs in the
future. However I made changes less intrusive only where it was easy to do.
Upstreaming will need more work on that.

I'll continue today with results of testing of the branch.

@Totktonada
Copy link
Member

I installed the updated version of tarantool into the system (from ebuild on
Gentool Linux) and performed the following tests in an empty directory:

  • tarantoolctl rocks search [substring-of-pkg-name]
  • tarantoolctl rocks help {<empty>|help|install}
  • install / remove avro-schema, vshard, mysql with and without a version
    specified; run tarantool and require installed modules
  • tarantoolctl rocks list [substring-of-pkg-name]
  • tarantoolctl rocks show avro-schema
  • tarantoolctl rocks doc avro-schema
  • tarantoolctl rocks make_manifest in a directory with .rocks directory
  • tarantoolctl rocks pack avro-schema creates
    avro-schema-scm-1.linux-x86_64.rock file.
  • tarantoolctl rocks unpack avro-schema-scm-1.linux-x86_64.rock creates
    avro-schema-scm-1 directory with the following content:
avro-schema-scm-1/
├── avro-schema-scm-1.rockspec
├── doc
│   ├── CHANGELOG.md
│   └── README.md
├── lib
│   └── avro_schema_rt_c.so
├── lua
│   └── avro_schema
│       ├── backend.lua
│       ├── compiler.lua
│       ├── fingerprint.lua
│       ├── frontend.lua
│       ├── il.lua
│       ├── init.lua
│       ├── runtime.lua
│       └── utils.lua
└── rock_manifest

I don't know how to verify whether it is good or bad.

  • tarantoolctl rocks unpack vshard creates vshard-scm-1/vshard directory
    with content of the repository. The layout is different from unpack of a
    compiled rock file.
  • Installed avro-schema, packed it (compiled rock), removed avro-schema, then
    tarantoolctl rocks install avro-schema-scm-1.linux-x86_64.rock.

Everything looks good except the following notes:

  • tarantoolctl rocks help shows options which are not supported for now. We
    however should support passing options to rocks subcommands and I propose to
    do so in the scope of #3632.
  • tarantoolctl rocks doc avro-schema opens README.md (saved locally) in a
    browser. Firefox doesn't want to open it directly, but rather ask me to
    download it. We possibly should provide another way to open a document.
  • I cannot verify results of unpack command, because don't know how should a
    layout look.

Then I step into the directory with clone avro-schema module and made the
following tests:

  • Slightly changed the module and typed tarantoolctl rocks make; verified
    that the changed module is installed into the current directory (to .rocks).
  • tarantoolctl rocks pack avro-schema-scm-1.rockspec creates
    avro-schema-scm-1.src.rock file.
  • tarantoolctl rocks unpack avro-schema-scm-1.src.rock creates
    avro-schema-scm-1 directory with the following content:
avro-schema-scm-1
├── avro-schema
│   └─ <repository content>
└── avro-schema-scm-1.rockspec
  • tarantoolctl rocks install avro-schema-scm-1.src.rock installs the module
    into .rocks.
  • Applied the following diff on avro-schema repo:
diff --git a/avro-schema-scm-1.rockspec b/avro-schema-scm-1.rockspec
index ddb020b..c995d8c 100644
--- a/avro-schema-scm-1.rockspec
+++ b/avro-schema-scm-1.rockspec
@@ -1,3 +1,4 @@
+rockspec_format = '3.0'
 package = 'avro-schema'
 version = 'scm-1'
 source  = {
@@ -26,4 +27,8 @@ build = {
         TARANTOOL_INSTALL_LUADIR="$(LUADIR)",
     },
 }
+test = {
+    type = 'command',
+    command = 'cmake . && make && make check',
+}
 -- vim: syntax=lua

Run tarantoolctl rocks test, tests are passed: the exit code is 0. Broke one
test, run again: the exit code is 1.

Everything looks good except maybe:

  • I don't know whether the layout produced by unpack is right or not.

To sum up: in my installation the patchset seems to work as expected.

@Totktonada
Copy link
Member

As Yarosval observed, the problem that is workarounded by the 'Fix concatenate with a nil value when give a warn' patch is already fixed in upstream ( luarocks@daf7e7e ) in more general way. We discussed it and decided to proceed now with our workaround and remove it when we'll prepare update to the new luarocks version.

@Totktonada
Copy link
Member

Re 'strange string.match behaviour':

We debugged it with Yaroslav. It was due to interpreting ??) as the trigraph for ] ( see https://gcc.gnu.org/onlinedocs/cpp/Initial-processing.html ) when luarocks is bundled into tarantool. We'll fix it in tarantool with escaping of question marks in extra/txt2c.c.

@rosik
Copy link

rosik commented Jun 16, 2019

I've run some tests on static layout (tarantool is built statically, installed in home directory).

  1. Warning: Could not find a Lua interpreter for version 5.1 is still issued because tarantool isn't located in hardcoded.PREFIX. I guess it's not a blocker and we can skip it later.

  2. The problem, which was solved by 'Fix strange string.match incorrect behavior' (f4e2b31) was caused by built-in luarocks.
    In our environment it was converted to a C file with static string, which contained a trigraph ??)/

GCC issued a warning:

/tarantool/src/../third_party/luarocks/src/luarocks/util.lua.c:715:61: warning:
                                                 trigraph ??) converted to ] [-Wtrigraphs]
 "         local vo, optional = valid_opts[k]:match(\"^(.-)(%??)$\")\n"
                                                             ^

We decided to fix it later here: https://github.com/tarantool/tarantool/blob/master/extra/txt2c.c

  1. I've tested commands make, install, doc, list, show, pack, search and they seem to work as expected.

@rosik
Copy link

rosik commented Jun 16, 2019

  1. tarantoolctl rocks test seems to work too.

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

It seems the patchset handles all needs of the standard tarantool layout (when it is installed from a package manager). Portable layout or using tarantoolctl rocks directly from a build directory can lead to a warning re unability to find a Lua interpreter (it is not an error). We will handle it later I think.

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

4 participants