pemsley / coot

Software for macromolecular model-building
http://www2.mrc-lmb.cam.ac.uk/personal/pemsley/coot/
GNU General Public License v3.0
114 stars 45 forks source link

Official Debian package #120

Open alexmyczko opened 4 months ago

alexmyczko commented 4 months ago

There's the intent to package at https://bugs.debian.org/897673 and the current public state: https://salsa.debian.org/science-team/coot and then there's some success on a local machine that builds 1.1.07 and if software builds successfully, it's likely to also work and be packagable... (Using this place to keep track of the history of building the package)

correction: almost success

[100%] Linking CXX executable test-molecules-container
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/libssm.so: undefined reference to `mmdb::Atom::Transform(double (&) [4][4])'
/usr/bin/ld: /usr/lib/x86_64-linux-gnu/libssm.so: undefined reference to `mmdb::Mat4Copy(double (&) [4][4], double (&) [4][4])'
collect2: error: ld returned 1 exit status

Just to see where we are: https://repology.org/project/coot/versions (comparisons are bad, Paul Watzlawik, Situation is Hopeless, But Not Serious, The Pursuit of Unhappiness)

pemsley commented 2 months ago

Re: the wrappers. It seems to me that they can all be replaced by one-liners that wraps the executable in libexec.

pemsley commented 2 months ago

What is needed for the man pages?

merkys commented 2 months ago

cootilus/nautilus_lib.pdb is a PDB file, not source code. How does the license work for that (there are other such files, for example cootaneer/cootaneer-llk-2.40.dat, protein_db/protein.db).

PDB files originating from the Protein DataBank are licensed under PDB license. Other PDB files, just as any other non-code files, are usually thought as licensed under the terms of top-level COPYING file.

Also cootaneer/Makefile-coot - that also doesn't have a copyright notice. It is not used, I think. Should I remove it?

As said in comment above, all the files which have no explicit copyright notices and originate from the same project are usually deemed licensed under the same terms of top-level COPYING file.

This is slow-going, painstaking and dull.

I understand. Thank you very much for clarifying copyright details. I believe this will be beneficial for distributions other than Debian as well, at least for Fedora which as well pays careful attention to copyright details.

Re: the wrappers. It seems to me that they can all be replaced by one-liners that wraps the executable in libexec.

That is exactly my idea.

What is needed for the man pages?

In Debian every executable in the main namespace should (not a hard "must") have a manpage. The detail level in manpages range from merely a sentence-or-two about the executable to very detailed instructions including examples and troubleshooting. Any point within this spectrum is OK. If the executable is a CLI tool, I personally like when its manpage describes CLI options (I teach my students to read manpages before turning to the Web for help). I think it is important to keep manpages up-to-date.

Origins of manpages vary:

  1. some manpages are contributed by upstreams,
  2. Some are automatically converted from some other pieces of documentation,
  3. some are automatically generated from --help output of an executable by help2man
  4. some are written by Debian maintainers.

I prefer options 1-3, as 4 is hardly sustainable. Is there something we could reuse for coot?

merkys commented 2 months ago

I just noticed man/ directory. It seems to contain manpages for most of the executables - great!

pemsley commented 2 months ago

OK, I think I've cleaned up every directory except src.

pemsley commented 2 months ago

OK! src directory done. Over to you. The files should be "grepable" now.

Not sure how to deal with pyrogen/jay_utils.py

pemsley commented 2 months ago

OK, it seems to me that I need to add man pages for layla, pyrogen and coot-identify-protein. I can probably get that done tomorrow.

merkys commented 2 months ago

OK! src directory done. Over to you. The files should be "grepable" now.

Awesome, thanks a lot! I will get to it ASAP.

Not sure how to deal with pyrogen/jay_utils.py

Content from Stack Overflow is CC-BY-SA, which version depends on the date of the post. I have not tried, but maybe some search engine will locate the original post.

pemsley commented 2 months ago

Here it is: https://stackoverflow.com/questions/377017/test-if-executable-exists-in-python

merkys commented 2 months ago

Thanks - I believe this is the last piece I needed to finish debian/copyright. I did most of the work yesterday and I hope to finish today/early next week. I have already done the wrappers, thus I am going to submit the package for copyright audit as soon as I finish debian/copyright. We can solve the remaining issues in the following weeks.

merkys commented 2 months ago

OK, debian/copyright is done, you may see it using the link. I am proceeding towards submitting the package for the copyright review.

alexmyczko commented 2 months ago

@merkys that means upload for https://salsa.debian.org/ftp-team/manpages/-/blob/master/doc/ftp-new.7.man

merkys commented 2 months ago

@merkys that means upload for https://salsa.debian.org/ftp-team/manpages/-/blob/master/doc/ftp-new.7.man

Yes, in Debian parlance it is just an upload to NEW queue.

pemsley commented 2 months ago

Reading the above - hats off to the reviewer on the other side of the fence. It seems like a big job.

Also MoleculesToTriangles/CXXClasses has its own COPYING - I should remove that I think - Don't you?

merkys commented 2 months ago

Reading the above - hats off to the reviewer on the other side of the fence. It seems like a big job.

Absolutely. They have to deal with hundreds of packages every month, and this is a rather thankless albeit voluntary work.

Also MoleculesToTriangles/CXXClasses has its own COPYING - I should remove that I think - Don't you?

It may stay, indicating that most of the files under MoleculesToTriangles/ are licensed under LGPL-3. But also since probably every LGPL-licensed file has LGPL-3 header, this COPYING file is redundant. I do not have a strong opinion here.

pemsley commented 2 months ago

This COPYING is for the GNU GPL-3.

merkys commented 2 months ago

This COPYING is for the GNU GPL-3.

Sorry, I looked at MoleculesToTriangles/LICENSE instead, although you were clearly referring to MoleculesToTriangles/CXXClasses/COPYING. Yes, MoleculesToTriangles/CXXClasses/COPYING seems redundant to me.

pemsley commented 2 months ago

Yes, MoleculesToTriangles/CXXClasses/COPYING seems redundant to me

Done.

alexmyczko commented 2 months ago

test build on arm64 succeeded: http://bananas.debian.net/debian/coot/

pemsley commented 2 months ago

Let's talk about debian/patches

  1. I have applied the coot.desktop patch.
  2. tell me about removing libdw and libelf - the idea was that the backtrace will help me identify bug (in combination with backward.hpp)
  3. dynarama-main.patch is a strange name for what the patch does.
  4. Is the binary supposed to be relocatable? I think you can get rid of coot.in and replace it with:
    x=$(readlink -f $0)
    d=$(dirname $x)
    exec $d/../libexec/coot-bin "$*"
alexmyczko commented 2 months ago

also a simple sid backport (needed gemmi and nanobind) to debian 12 (bookworm) is no problem. more of a problem with ubuntu 22.04 (gtk4).

merkys commented 2 months ago

Let's talk about debian/patches

Right. Patches coot.desktop.patch, dynarama-main.patch and coot-paths.patch are not used for quite some time. They are leftovers from earlier built attempts and are commented out in debian/patches/series which lists all applied patches in the patching order. I have just removed them completely.

  1. I have applied the coot.desktop patch.

Thanks - I have left it unapplied, but lintian (Debian package checker) was complaining about the missing placeholder in Exec field.

  1. tell me about removing libdw and libelf - the idea was that the backtrace will help me identify bug (in combination with backward.hpp)

Right, I was meaning to talk about this. Instruction find_library(libdwarf dw) fails on Debian as libdwarf has neither .cmake nor .pc file. Since in Debian it is quite unusual to link end-user binaries with libdw and libelf, I commented them out. I understand now why they are useful and if you think coot binaries in Debian should have built-in debugging capabilities, I can look for a way to build with them. Otherwise there could be a build variable that would turn on the debug build.

  1. dynarama-main.patch is a strange name for what the patch does.

This patch is not used, removed.

  1. Is the binary supposed to be relocatable? I think you can get rid of coot.in and replace it with:
x=$(readlink -f $0)
d=$(dirname $x)
exec $d/../libexec/coot-bin "$*"

No, it does not have to be relocatable. I retained the original wrappers with just a minimal change to LD_LIBRARY_PATH.

@alexmyczko thanks for looking into non-amd64 builds and backporting!

pemsley commented 2 months ago

find_library(libdwarf dw) is not so much of a concern at the moment - that is for the libcootapi/chapi build. So I don't have strong feelings about that. If I get bug reports then I might change my mind. I notice that Caliper has a FindLibDw.cake:

https://github.com/LLNL/Caliper/blob/master/cmake/FindLibDw.cmake

That could be added along with FindCairo.cmake.

For the Coot build, you should be passing --with-libdw to configure. That does a PKG_CHECK_MODULES.

OK, if the coot shell script wrapper works, let's not touch it for now.

pemsley commented 2 months ago

You might also consider --with-sound (but it doesn't work well at the moment).

merkys commented 2 months ago

I have asked the maintainer of Debian dwarfutils package to provide its .cmake file if possible (#1068626). If it gets provided then there will be no need to add FindLibDw.cmake to coot (at least for Debian builds).

I will attempt to use --with-libdw and --with-sound.

merkys commented 2 months ago

I was wrong about the failure of find_library(libdwarf dw) - I apparently confused libdwarf package with libdw. The latter seems to work. Thus I will drop the patch disabling libdw and libelf.

pemsley commented 2 months ago

I apparently confused libdwarf package with libdw

Oh? They aren't the same thing? Then so did I - haha!

merkys commented 2 months ago

Oh? They aren't the same thing? Then so did I - haha!

libdw apparently comes from elfutils and libdwarf from dwarfutils. I never had to deal with them, cannot really say what is the difference, but they are different :)

Build with --with-sound succeeds, thus from now on I am building with it.

pemsley commented 2 months ago

This is what I read: https://github.com/bombela/backward-cpp This is what I get from configure: LIBDW_LIBS='-ldw -lelf'

merkys commented 2 months ago

This is what I read: https://github.com/bombela/backward-cpp

Nice, thanks.

pemsley commented 2 months ago

OK, man pages for layla, pyrogen and coot-identify-protein have been added.

merkys commented 2 months ago

OK, man pages for layla, pyrogen and coot-identify-protein have been added.

Thanks.

On my end, I have tested coot and pyrogen executables. I ensured coot successfully reads in MTZ files, downloads entries from the PDB and COD.

pemsley commented 2 months ago

and COD

very important test :-)

This is a side-question: Is is possible to download the structure factors from COD structures (so that Coot can make a map?)

merkys commented 2 months ago

and COD

very important test :-)

Indeed :)

By the way, currently coot makes requests to the COD using CODSESSION URL parameter, although it is optional for data requests. Were there any problems accessing the COD without CODSESSION?

This is a side-question: Is is possible to download the structure factors from COD structures (so that Coot can make a map?)

The COD has structure factors in CIF HKL format, for example, 1100908.hkl. Some CIF files have embedded SHELX HKL structure factors, but we have not implemented parsing them yet. There seems to be a Python package ShelXFile which can read them.

pemsley commented 2 months ago

I am pretty sure I wrote a SHELX data parser - in 2006-2007 or so. Who knows where it is now though⸮ ShelXFile looks interesting - thanks.

I don't recall anything about CODSESSION.

pemsley commented 2 months ago

OK, where are we now? Are you waiting for me?

merkys commented 2 months ago

I am pretty sure I wrote a SHELX data parser - in 2006-2007 or so. Who knows where it is now though⸮ ShelXFile looks interesting - thanks.

We at the COD have plans to parse SHELX data embedded in CIF files, but so far we did not to anything about it. At some point we will have to investigate the existing SHELX parsers, it would be interesting to look into yours.

I don't recall anything about CODSESSION.

Here it is: https://github.com/pemsley/coot/blob/a494e2b32a527d2365344d3bd2edeeac21ba079a/src/c-interface-network.cc#L678 I would suggest switching to https:// and removing ?CODSESSION=fromCoot as it is not necessary.

OK, where are we now? Are you waiting for me?

I have submitted the package for copyright review a week two weeks ago. We may have to wait some more to finally see it in Debian. This is the single blocker for now. Meanwhile I have tested the package and fixed a bunch of packaging issues (mostly introduced by me).

It would be nice if at some point you could make a release with all the recent changes to the codebase. For now I have packaged a development version (from some git commit), but it would be nicer to have a stable release packaged for Debian.

pemsley commented 2 months ago

OK, I'll get a new release done on Monday.

pemsley commented 2 months ago

I would suggest switching to https:// and removing ?CODSESSION=fromCoot as it is not necessary.

Done d599bb957779cebc12a83fd0753b11a050d59039

alexmyczko commented 1 month ago

thank you https://buildd.debian.org/status/package.php?p=coot

merkys commented 1 month ago

Coot has passed the copyright checks on Saturday and has been accepted to Debian! Thanks a lot for making this happen!

The build matrix looks quite good. Some of the Debian's officially supported 64bit architectures are failing, but I have a feeling this might be due to incomplete time_64 transition. Anyway these are not a big deal.

My next step is to update the package to v1.1.08.

pemsley commented 1 month ago

When running autoconf:

From https://buildd.debian.org/status/fetch.php?pkg=coot&arch=hurd-i386&ver=1.1.07.705.gb7e2c16a2%2Bdfsg-1&stamp=1716577264&raw=0 ./get-the-git-commit-count.sh: line 1: git: command not found

It is not available? That doesn't seem right? Is the path wrong or something? I don't understand what's going on here - where is git on hurd-i386?

pemsley commented 1 month ago

Coot has passed the copyright checks on Saturday and has been accepted to Debian

Woohoo - fantastic news.

merkys commented 1 month ago

When running autoconf:

From https://buildd.debian.org/status/fetch.php?pkg=coot&arch=hurd-i386&ver=1.1.07.705.gb7e2c16a2%2Bdfsg-1&stamp=1716577264&raw=0 ./get-the-git-commit-count.sh: line 1: git: command not found

It is not available? That doesn't seem right? Is the path wrong or something? I don't understand what's going on here - where is git on hurd-i386?

git is not installed on any of the Debian build machines by default. Usual practice is to build from source tarballs, and these do not normally ship .git directory. Therefore I did not add git as build dependency for Coot.

As for hurd-i386, I think the problem might be with float comparison in gemmi:

/usr/include/gemmi/elem.hpp:141:48: error: static assertion failed: Hmm
  141 |   static_assert(radii[static_cast<int>(El::D)] == 0.31f, "Hmm");
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
/usr/include/gemmi/elem.hpp:141:48: note: the comparison reduces to ‘(3.10000002384185791016e-1l == 3.10000000000000000002e-1l)’
pemsley commented 1 month ago

float comparison in gemmi

That issue has (now) been brought up on our local discussion forum. Maybe I should have used GitHub.

merkys commented 1 month ago

What is the code trying to do? Cast to an int then compare to a float? (rhetorical question). I'm confused/surprised that it compiled at all anywhere.

I think El::D is cast to int and then used to access an element in an array of float values, which seems legit. Float comparison is tricky. It seems rounding up to ~3 decimal spaces should do the trick. Let us ask Gemmi developers, they usually react pretty fast.

pemsley commented 1 month ago

(I missed the [])

merkys commented 1 month ago

I have opened https://github.com/project-gemmi/gemmi/issues/316 to ask about the possible float comparison issue.

pemsley commented 1 month ago

Should I add a workaround for when git is not available when autogen.sh is run?

merkys commented 1 month ago

Should I add a workaround for when git is not available when autogen.sh is run?

This would be really nice to have.