Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#49058 closed enhancement (fixed)

apbs: several minor enhancements

Reported by: dstrubbe (David Strubbe) Owned by: howarth.at.macports@…
Priority: Normal Milestone:
Component: ports Version: 2.3.4
Keywords: haspatch Cc:
Port: apbs

Description (last modified by dstrubbe (David Strubbe))

Here is a patch for the Portfile to fix the following issues, for the maintainer's consideration.

Attachments (7)

Portfile-apbs.diff (1.5 KB) - added by dstrubbe (David Strubbe) 8 years ago.
Portfile-apbs.diff.2 (3.3 KB) - added by dstrubbe (David Strubbe) 8 years ago.
main.log_yosemite (198.1 KB) - added by dstrubbe (David Strubbe) 8 years ago.
Portfile.diff (2.8 KB) - added by howarth.at.macports@… 8 years ago.
Portfile to build apbs against gcc5's fortran
Portfile-apbs.diff.3 (3.9 KB) - added by dstrubbe (David Strubbe) 8 years ago.
Portfile-apbs.diff.4 (3.3 KB) - added by dstrubbe (David Strubbe) 8 years ago.
Portfile-apbs.diff.5 (4.6 KB) - added by dstrubbe (David Strubbe) 8 years ago.

Download all attachments as: .zip

Change History (29)

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: Portfile-apbs.diff added

comment:1 Changed 8 years ago by dstrubbe (David Strubbe)

Description: modified (diff)

comment:2 Changed 8 years ago by howarth.at.macports@…

In fink, the apbs packaging uses the url https://codeload.github.com/Electrostatics/apbs-pdb2pqr/zip/d4e78c62e6a07f92c26924318d83a159fa6af063 and the source directory apbs-pdb2pqr-d4e78c62e6a07f92c26924318d83a159fa6af063/apbs to build 1.4.1. However I am unclear how to get MacPorts to use such a download url. Note that fink has a SourceRename field which allows the downloaded source archive to be renamed to apbs-pdb2pqr-1.4.1.zip for extraction.

Last edited 8 years ago by howarth.at.macports@… (previous) (diff)

comment:3 Changed 8 years ago by dstrubbe (David Strubbe)

You can set it in master_sites. Also the github portgroup might be useful for this. The name of the resulting folder after extraction can be set with worksrcdir.

Incidentally, is there a compelling reason for apbs-mpi to be a subport as opposed to just a variant, like apbs +mpi? That is the way every other port using MPI I have come across is set up.

comment:4 Changed 8 years ago by howarth.at.macports@…

It appears that the current implementation of git support can't handle the directory configuration in upstream apbs-pdb2pqr. Using...

worksrcdir              ${name}-pdb2pqr/${name}
git.url                 https://github.com/Electrostatics/apbs-pdb2pqr.git
git.branch              d4e78c62e6a07f92c26924318d83a159fa6af063

produces a git pull with ${name}-pdb2pqr/${name}/${name}

That is you need to have the complete pull but need to build from within the ${name} subdirectory. The act of setting worksrcdir to ${name}-pdb2pqr/${name} causes the git pull to occur within the ${name} subdirectory rather than just treating that as the worksrcdir *AFTER* the git pull.

comment:5 Changed 8 years ago by dstrubbe (David Strubbe)

Hm, why don't you ask on the devel list about how to make this work?

comment:6 Changed 8 years ago by dstrubbe (David Strubbe)

Well, are you happy with my patch to start with? I can commit this in the meantime while you look for how to update to 1.4.1 properly.

comment:7 Changed 8 years ago by dstrubbe (David Strubbe)

Initially proposed changes committed in r141727. New Portfile attached that updates to 1.4.1 and uses the github portgroup. Note that you were not using the github portgroup in the lines you mentioned above.

Remove unused C flags line which has spurious "i" in it, and is overwritten anyway. Also BEM has to be disabled. Add test phase (a few tests fail for me). Add livecheck.

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: Portfile-apbs.diff.2 added

comment:8 Changed 8 years ago by dstrubbe (David Strubbe)

Also I added openmaintainer in this patch, in accordance with the suggestion in the Guide: "Port maintainers who are not committers are encouraged to add <openmaintainer> to their ports."

comment:9 Changed 8 years ago by dstrubbe (David Strubbe)

Resolution: fixed
Status: newclosed

Committed in r142208.

comment:10 Changed 8 years ago by howarth.at.macports@…

This commit ( r142208) broke the ability to build apbs.

https://trac.macports.org/ticket/49620

Please fix or revert.

ps Did anyone actually test this before committing?

Last edited 8 years ago by howarth.at.macports@… (previous) (diff)

comment:11 Changed 8 years ago by howarth.at.macports@…

I don't see how this packaging can possibly work. It is using a nasty hack of...

# make sure this comes after further options that the cmake portgroup adds, to specify directory correctly
configure.post_args     ./apbs

This changes appears insufficient to properly change the build directory throughout the build. If you solve the missing header issue with...

--- Portfile.orig	2015-11-08 12:30:50.000000000 -0500
+++ Portfile	2015-11-09 19:54:42.000000000 -0500
@@ -5,6 +5,7 @@
 PortGroup               cmake 1.0
 PortGroup               mpi 1.0
 PortGroup               github 1.0
+PortGroup               compilers 1.0
 
 compilers.choose        cc cxx
 
@@ -32,17 +33,19 @@
 checksums               rmd160  5c4d583e12deb3fbc2b5a8031882311cbfd22c7e \
                         sha256  f98ce6a51d8f813e1b4fa626c054ddbf7a985403ca30f890733cb1abf2bd6e05
 
-depends_lib             port:maloc \
+depends_lib             port:eigen3 \
+                        port:maloc \
                         port:readline
 
-# BEM needs 'gfortran', hard-coded, and configure fails without it if BEM is enabled, with a syntax error
-#CMake Error at CMakeLists.txt:217 (get_filename_component):
-#  get_filename_component called with incorrect number of arguments
+compilers.setup         require_fortran -clang -dragonegg -gcc44 -gcc45 -gcc46 -g95
+
 configure.args-append   -DENABLE_OPENMP:BOOL=OFF \
-                        -DENABLE_BEM=OFF
+                        -DENABLE_BEM=OFF \
+                        -DCMAKE_CXX_COMPILER_ARG1:STRING="-I${prefix}/include -I${prefix}/include/eigen3" \
+                        -DCMAKE_C_COMPILER_ARG1:STRING="-I${prefix}/include -I${prefix}/include/eigen3" 
 
 # make sure this comes after further options that the cmake portgroup adds, to specify directory correctly
-configure.post_args     ./apbs
+configure.post_args     ./apbs                        
 
 test.run  yes
 test {

You are still left with the build being unable to properly find the built libraries...

ld: library not found for -lapbs_geoflow

The packaging changes required for 1.4.1 (which has dramatic changes in the source layout are very far from ready). Please revert this r142208 until these changes are properly tested before committng.

Version 1, edited 8 years ago by howarth.at.macports@… (previous) (next) (diff)

comment:12 Changed 8 years ago by howarth.at.macports@…

The whole approach being proposed in this ticket seems very dubious to me and I don't believe it will work properly. You are basically executing cmake from a directory *above* the source directory for apbs and them later executing make from that some elevated directory. IMHO, this is bound to be highly untested and exposing cmake bugs.

Again please revert this packaging.

Last edited 8 years ago by howarth.at.macports@… (previous) (diff)

comment:13 Changed 8 years ago by dstrubbe (David Strubbe)

Hi, please relax a little bit. It is absolutely tested, I tried numerous times on my machine with Yosemite and it worked fine and passed all the tests (for which I added support). Also, I did not see any error on the buildbots. You had the opportunity over the last 3 weeks to try it yourself too and give feedback, but you didn't give any comment.

I guess there is a problem that appears under some other circumstance or on a different OS. For me, eigen3 is not used and it builds fine without it. I'm not sure why this would be different, but that is the question that needs to be answered to resolve this problem. Are you able to reproduce the problem reported in the other ticket?

I have seen other Portfiles using cmake that have this post_args, and have had it for a long time; it is a documented feature of cmake and is certainly not a nasty hack. Look at https://cmake.org/cmake/help/v2.8.8/cmake.html#section_Usage: usage = cmake [options] <path-to-source>. That's what we're doing here. I am not sure what your concern is about it.

comment:14 Changed 8 years ago by dstrubbe (David Strubbe)

You can compare this to the posted main.log in the other ticket and see if you can understand why they are different.

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: main.log_yosemite added

comment:15 Changed 8 years ago by Ionic (Mihai Moldovan)

Yeah, we determined on IRC that it doesn't seem to be causing problems per se, but configuring into a topdir is weird still.

comment:16 Changed 8 years ago by howarth.at.macports@…

The attached Portfile diff builds the BEM support on my darwin15 machine as lone as I move aside the /opt/local/bin/as from cctools. Other wise I get the failures...

/opt/local/var/macports/build/_Users_howarth_ports_science_apbs/apbs/work/.tmp/cc6Ra1ry.s:6201:suffix or operands invalid for `movq' /opt/local/var/macports/build/_Users_howarth_ports_science_apbs/apbs/work/.tmp/cc6Ra1ry.s:6285:suffix or operands invalid for `movq' /opt/local/var/macports/build/_Users_howarth_ports_science_apbs/apbs/work/.tmp/cc6Ra1ry.s:6289:suffix or operands invalid for `movq' /opt/local/var/macports/build/_Users_howarth_ports_science_apbs/apbs/work/.tmp/cc6Ra1ry.s:6623:suffix or operands invalid for `movq' /opt/local/var/macports/build/_Users_howarth_ports_science_apbs/apbs/work/.tmp/cc6Ra1ry.s:6627:suffix or operands invalid for `move'

Changed 8 years ago by howarth.at.macports@…

Attachment: Portfile.diff added

Portfile to build apbs against gcc5's fortran

comment:17 Changed 8 years ago by dstrubbe (David Strubbe)

Why do we need this? "use_parallel_build no"

I'm not sure what your previous comment about BEM means: "move aside the /opt/local/bin/as from cctools". Is that something which is happening in the Portfile?

comment:18 Changed 8 years ago by dstrubbe (David Strubbe)

Ok, I see that the parallel build apparently is responsible for the failures to find the files for Eigen (internal, not from eigen3 port) and apbs_geoflow, evidently due to some bugs in the APBS build system.

This part, and your preferred way of managing the directory structure, committed in r142365. (Let me point out that the cmake portgroup always specifies the directory explicitly in the configure step anyway. Take a look at the command it executes, which will end in ${worksrcpath}.)

Your fix for BEM is a good start but insufficient: the build automatically detects a Fortran compiler, not necessarily gcc5. In my case it found and used g95, which failed to link with the gcc5 library not surprisingly. More patching is needed.

The lines you added about include paths have no effect: "-I${prefix}/include" is the default so it's already used; "-I${worksrcpath}/include" does not exist as a path. Such things (when needed) are really supposed to go into CFLAGS not the C compiler command directly, anyway.

Since we are including the compilers portgroup, we should use it for support for Fortran, rather than doing it by hand. It is typical to create a variant in a situation like this for BEM, since it introduces an additional dependency.

Last edited 8 years ago by dstrubbe (David Strubbe) (previous) (diff)

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: Portfile-apbs.diff.3 added

comment:19 Changed 8 years ago by dstrubbe (David Strubbe)

Try this out. I think it enables BEM reliably. It depends on something I just added to the compilers portgroup, so be sure you do selfupdate before trying it. Also I blacklisted various compilers that would not work.

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: Portfile-apbs.diff.4 added

comment:20 Changed 8 years ago by dstrubbe (David Strubbe)

A few more things fixed up with compilers in this 4th revision.

comment:21 Changed 8 years ago by dstrubbe (David Strubbe)

Solve ticket #48544 too with this one. (Linking with readline; not clear why it works without that sometimes or without MPI!)

Last edited 8 years ago by dstrubbe (David Strubbe) (previous) (diff)

Changed 8 years ago by dstrubbe (David Strubbe)

Attachment: Portfile-apbs.diff.5 added

comment:22 Changed 8 years ago by dstrubbe (David Strubbe)

Committed r149254.

Note: See TracTickets for help on using tickets.