Skip to content
Snippets Groups Projects

Pre-2.0 cleanup

Merged Petr Špaček requested to merge cleanup into master

This cleanup breaks backwards compatibility in less used features, but I think that it is acceptable for the upcoming 2.0 release.

Merge request reports

Pipeline #26936 passed

Pipeline passed for fdbce86b on cleanup

Test coverage 65.60% (-0.10%) from 1 job
Approval is optional

Merged by Petr ŠpačekPetr Špaček 7 years ago (Dec 1, 2017 1:27pm UTC)

Merge details

  • Changes merged into with aaf98192.
  • Deleted the source branch.

Pipeline #26941 passed

Pipeline passed for aaf98192 on master

Test coverage 65.80% (-0.10%) from 1 job

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • merged

  • Petr Špaček mentioned in commit aaf98192

    mentioned in commit aaf98192

  • @vavrusam might remember the reason for module naming. I see we first do dlopen on absolute paths, so that should be OK, but the next phase – loading a lua module might be prone to collisions with same-named files (etcd.lua does exist upstream). Off the top of my head, I'm not certain about precedence in lua.

  • Author Contributor

    Hmm, I did not realize that our etcd module depends on different etcd module. Luckily the other module is named etcd.luasocket so loading should still work.

    Module load order seems correct even if we are loading "submodule" below an existing parent (io).

    modules.load('io.blablabla')
    error: module 'io.blablabla' not found:
    	no field package.preload['io.blablabla']
    	no file '/usr/local/lib/kdns_modules/io/blablabla.lua'
    	no file '/usr/local/lib/kdns_modules/io/blablabla/init.lua'
    	no file './io/blablabla.lua'
    	no file '/usr/share/luajit-2.1.0-beta3/io/blablabla.lua'
    	no file '/usr/local/share/lua/5.1/io/blablabla.lua'
    	no file '/usr/local/share/lua/5.1/io/blablabla/init.lua'
    	no file '/usr/share/lua/5.1/io/blablabla.lua'
    	no file '/usr/share/lua/5.1/io/blablabla/init.lua'
    	no file '/usr/local/lib/kdns_modules/io/blablabla.so'
    	no file './io/blablabla.so'
    	no file '/usr/local/lib/lua/5.1/io/blablabla.so'
    	no file '/usr/lib64/lua/5.1/io/blablabla.so'
    	no file '/usr/local/lib/lua/5.1/loadall.so'
    	no file '/usr/local/lib/kdns_modules/io.so'
    	no file './io.so'
    	no file '/usr/local/lib/lua/5.1/io.so'
    	no file '/usr/lib64/lua/5.1/io.so'
    	no file '/usr/local/lib/lua/5.1/loadall.so'
    error: No such file or directory

    I will do couple experiments to make sure it works... Also, we should do thought-experiment to find out whether our Lua modules should loaded using require('kdns_modules.<module_name>') to avoid conflicts with global namespace... or maybe kres_modules so it is unique and not possibly mixed up with Knot DNS.

    BTW right now modules.load('io') silently succeeds and I do not like this kind of surprises.

  • Hmm, I did not realize that our etcd module depends on different etcd module.

    It doesn't, but a lua module of that name does exist on luarocks. I agree it will be better to put our modules into some distinct namespace.

  • I think the reason was to avoid collision with the packages used by modules (kmemcached uses memcached etc). It'd be nicer solution to use the namespace as you said, that way modules can either do modules.load 'abcd' or require('kdns_modules.abcd'). The built-in packages (like kres) should be includable without prefix though.

  • mentioned in issue #279 (closed)

Please register or sign in to reply