LeoIannacone / npm2deb

tool to help debianize Node.js modules
GNU General Public License v3.0
46 stars 34 forks source link

query madison API directly to find source package (Closes: #909753) #122

Closed anarcat closed 5 years ago

anarcat commented 5 years ago

The tool used by the JavaScript team to track progress on packaging, js_task_edit1, has trouble dealing with binary packages that have a different name than the source package. For example, in the OpenPGP packaging page2, a bunch of babel packages are marked as missing in Debian even though they are actually present (e.g. babel-code).

This is because of that bit of rmadison code in mapper.py:

    madison = _getstatusoutput(
        'rmadison -u debian "%s" | tac | grep source' % result['name'])

If you look at the output for babel-core, you'll see this can't possibly work:

$ rmadison -u debian node-babel-core
node-babel-core | 6.26.0+dfsg-3 | testing    | all
node-babel-core | 6.26.0+dfsg-3 | unstable   | all

The "-S" does not help here: it would show binary packages if we somehow magically found the right source package, but not the reverse. There's no way the commandline rmadison tool can give us that information, because that's a limitation of the API.

At least that's what I thought at first. As it turns out, there's an undocumented python output format hidden deep in the entrails of Dak3 which, in turn, actually indicate the source package associated with any binary package found. This nasty business requires us to do an actual HTTP query in Python, but rmadison does that anyways, so doing so is not really slower.

This patch fixes the problem: return the source package instead of the binary package and things all fall into place. As a bonus, we sort the version numbers with Python's distutils which should be more reliable than tac (if that's even what that thing was doing).

One downside is that this might return testing instead of unstable if both have the same version (because that's the first match madison returns, and why tac was used) but I would counter it's more interesting to find the older suite with the latest version than the later suite, as that's obviously always unstable. This gives more information about the state in Debian (e.g. it might even be stable if we're lucky!)

pravi commented 5 years ago

@anarcat see https://salsa.debian.org/js-team/npm2deb/merge_requests/1#note_45014 and lets discuss more here

anarcat commented 5 years ago

so this is what I mentioned in the last graf of the commitlog:

One downside is that this might return testing instead of unstable if both have the same version (because that's the first match madison returns, and why tac was used) but I would counter it's more interesting to find the older suite with the latest version than the later suite, as that's obviously always unstable. This gives more information about the state in Debian (e.g. it might even be stable if we're lucky!)

This is by design. I'm happy to learn the thing is in backports! It means the package will work even down in backports. Now, if the version in backports that shows up is lower than the version in unstable, that's another problem, and one we'd have to deal with possibly in other scenarios (e.g. security updates) anyways. This, specifically, comes up because Python's distutils.version does not know about Debian-specific version syntax like ~. I wonder if we could pull in python-apt to get such version comparison routines or if there's a better one that could handle this correctly?

pravi commented 5 years ago

This seems to be not a big issue, only cosmetic issue as can be seen in the example below. It is showing the highest upstream version.

pravi@nishumbha:~$ npm2deb view glob
Name:                                   glob
Version:                                7.1.3
Description:                            None
Homepage:                               https://github.com/isaacs/node-glob#readme
License:                                ISC
Debian:                                 node-glob (7.1.3-1) (testing)
pravi@nishumbha:~$ rmadison node-glob
node-glob  | 4.0.5-1~bpo70+1 | wheezy-backports   | source, all
node-glob  | 4.0.5-1         | oldstable          | source, all
node-glob  | 4.0.5-1         | oldstable-kfreebsd | source, all
node-glob  | 7.1.1-1         | stable             | source, all
node-glob  | 7.1.3-1         | testing            | source, all
node-glob  | 7.1.3-1         | unstable           | source, all
pravi commented 5 years ago

@anarcat further testing lead to this error.


 npm2deb create @ava/write-file-atomic
Traceback (most recent call last):
  File "/usr/bin/npm2deb", line 7, in <module>
    sys.exit(main(sys.argv))
  File "/usr/lib/python3/dist-packages/npm2deb/scripts.py", line 157, in main
    args.func(args)
  File "/usr/lib/python3/dist-packages/npm2deb/scripts.py", line 293, in create
    npm2deb.start()
  File "/usr/lib/python3/dist-packages/npm2deb/__init__.py", line 77, in start
    self.create_control()
  File "/usr/lib/python3/dist-packages/npm2deb/__init__.py", line 277, in create_control
    args['Depends'] = self._get_Depends()
  File "/usr/lib/python3/dist-packages/npm2deb/__init__.py", line 538, in _get_Depends
    name = mapper.get_debian_package(dep)['name']
  File "/usr/lib/python3/dist-packages/npm2deb/mapper.py", line 82, in get_debian_package
    if distutils.version.LooseVersion(vers) > result['version']:
  File "/usr/lib/python3.6/distutils/version.py", line 64, in __gt__
    c = self._cmp(other)
  File "/usr/lib/python3.6/distutils/version.py", line 337, in _cmp
    if self.version < other.version:
TypeError: '<' not supported between instances of 'int' and 'str'
anarcat commented 5 years ago

that's distutils freaking out on Debian's version numbering scheme. it tries to compare ":" with 1 in the version numbers and, obviusly, fails. a workaround is to avoid using an epoch in the default version number, but this goes back to our previous discussion about comparing debian version numbers correctly - there might be other cases this fails, unfortunately - especially with packages with epochs. :(

anarcat commented 5 years ago

the new commit fixes the immediate error, but we should find some better parser for debian version strings somewhere. python-apt's package.Version object could provide what we need, but i wonder if it's okay to add a new dependency...

pravi commented 5 years ago

I think python-apt would be fine to add.

anarcat commented 5 years ago

right - i renamed a few variables at some point and probably screwed it up... i'll factor in the python-apt depends so we get proper version comparison anyways and get back to you.

anarcat commented 5 years ago

alright, after a little bit of fighting with apt_pkg, i got it to work and we should have proper version comparisons now:

$ npm2deb view glob
Name:                                   glob
Version:                                7.1.3
Description:                            None
Homepage:                               https://github.com/isaacs/node-glob#readme
License:                                ISC
Debian:                                 node-glob (7.1.3-1) (testing)
$ npm2deb view 'write-file-atomic'
Name:                                   write-file-atomic
Version:                                2.3.0
Description:                            None
Homepage:                               https://github.com/iarna/write-file-atomic
License:                                ISC
Debian:                                 node-write-file-atomic (2.3.0-1) (testing)

i believe that, before the patch, the latter would have shown the version from backports (or crashed, i'm not sure). :) It would arguably have been great to have the backports version in there, but I think it's better to have a consistent and reliable codebase than rely on Python's incomplete comparison algorithms that might crash on Debian versions.

anarcat commented 5 years ago

:tada: