petscconf.h leaves PETSC_DIR set to builddir after installation

Issue #208 resolved
Drew Parsons
created an issue

petscconf.h contains a definition of PETSC_DIR which is set by the configure scripts to the value specified at configure time. If I read config/PETSc/options/petscdir.py, the value of PETSC_DIR given at configure time needs to be the build dir (the current directory).

But petscconf.h is also included in the final installation, and its value of PETSC_DIR is not updated. This means in the installation, petscconf.h sets PETSC_DIR to the builddir rather than the installdir.

So I think the value of PETSC_DIR in petscconf.h should be reset at install time.

I've worked around it in the debian installation by manually using sed to update petscconf.h after installation.

I've reported the analogous problem for SLEPc at https://bitbucket.org/slepc/slepc/issues/11/slepcconfh-leaves-slepc_dir-set-to

Comments (35)

  1. Drew Parsons reporter
    • changed status to open

    Thanks Barry. A 2-line patch, that does indeed show the power of the python install.

    I'd like to reopen for this reason: both installdir (prefix) and destdir are involved, and may not be the same location.

    The use-case is in packaging (e.g. creating Debian deb packages). We build in builddir with installdir as the target where PETSc will ultimately be hosted. But destdir is intermediary. We use "make install" to install in destdir, then organise the contents of destdir into a deb file that users use to install into installdir.

    With your patch, at install time I get

    make -j4 -O install DESTDIR=/build/petsc-3.8.3\+dfsg1/debian/tmp/petsc3.8-real-debug/usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug AM_UPDATE_INFO_DIR=no DESTDIR=debian/tmp/petsc3.8-real-debug//usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug PETSC_DIR=/build/petsc-3.8.3\+dfsg1 PETSC_ARCH=x86_64-linux-gnu-real-debug
    make[2]: Entering directory '/build/petsc-3.8.3+dfsg1'
    *** Using PETSC_DIR=/build/petsc-3.8.3+dfsg1 PETSC_ARCH=x86_64-linux-gnu-real-debug ***
    *** Copying PETSc to DESTDIR location: /build/petsc-3.8.3+dfsg1/debian/tmp/petsc3.8-real-debug/usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug  ***
    Traceback (most recent call last):
      File "./config/install.py", line 437, in <module>
        Installer(sys.argv[1:]).run()
      File "./config/install.py", line 432, in run
        self.runfix()
      File "./config/install.py", line 418, in runfix
        self.fixConf()
      File "./config/install.py", line 278, in fixConf
        self.fixConfFile(os.path.join(self.installIncludeDir,'petscconf.h'))
      File "./config/install.py", line 252, in fixConfFile
        oldFile = open(src, 'r')
    IOError: [Errno 2] No such file or directory: '/usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug/include/petscconf.h'
    

    Here

    builddir = /build/petsc-3.8.3+dfsg1
    destdir = /build/petsc-3.8.3+dfsg1/debian/tmp/petsc3.8-real-debug/usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug
    installdir (prefix) = /usr/lib/petscdir/petsc3.8/x86_64-linux-gnu-real-debug/
    

    I think the error might be the second line in the patch. Instead of

    self.fixConfFile(os.path.join(self.installIncludeDir,'petscconf.h'))
    

    we probably want something like

    self.fixConfFile(os.path.join(self.destIncludeDir,'petscconf.h'))
    
  2. BarryFSmith
    • changed status to open

    The previous fix produces errors if the path of the prefix location includes the path of the build PETSC_DIR since it gets overwritten in the text substitution. Plus several other variables are set wrongly after a prefix install.

    Therefor reverted this fix and @Satish Balay introduced a new branch balay/fix-includes-install that sets these variables correctly when ./configure is run instead of patching them at make install time.

    The branch has been added to next for testing. If it resolves the problem and the fix works for @Drew Parsons then this issue will be marked as resolved for the third time.

  3. Drew Parsons reporter

    A quick test of balay/fix-includes-install indicates it's fine as far as PETSC_DIR and petscconf.h goes. I noticed that PETSC_DIR in petscconf.h gets set to its final (prefix) value at configure time, rather than its builddir value.

    I think I've found another builddir problem however. In lib/pkgconfig/PETSc.pc,
    cflags_extra and fflags_extra both contain -fdebug-prefix-map=<builddir>.
    The same -fdebug flags show up in

    include/petscconfiginfo.h
    include/petscmachineinfo.h
    lib/pkgconfig/PETSc.pc
    lib/petsc/conf/reconfigure-x86_64-linux-gnu-real.py
    lib/petsc/conf/petscvariables
    lib/petsc/conf/RDict.db
    lib/libpetsc_real.so.3.08.3
    

    There are also other references to builddir in lib/petsc/conf/testfiles and lib/petsc/conf/uninstall.py

    Aside from those builddir references, I found that petsc/pkg-sowing was downloaded, which is unusual for a Debian build. But if I got the right sense of it, that's expected when building directly from PETSc git, rather than from the source tarball.

    I found also that the soname was set to libpetsc.so.3.08 rather than libpetsc.so.3.8. Is that expected for balay/fix-includes-install or would it be a side-effect of our Debian patches that I can fix at my end?

  4. Jed Brown
    1. Where is -fdebug-prefix-map coming from?

    2. testfiles is now created in a different place and not installed.

    3. uninstall.py contains a list of pairs (source, dest). I have no idea why, versus just a list of the dest.

    4. If you have a current version of sowing installed, then PETSc will not download it again. It is just a build dependency that PETSc fetches automatically when not found.

    5. The soname flags non-release builds with that leading 0. We provide ABI compatibility for releases (tagged on branch 'maint') but not for the development version. So libpetsc.so.3.8 ensures compatibility while there might be different ABI-incompatible versions with soname libpetsc.so.3.08. It probably doesn't make sense for a distribution to provide a libpetsc.so.3.08 unless it's very clear that it is a development version with no ABI commitment.

  5. Drew Parsons reporter

    1 . -fdebug-prefix-map comes from the CFLAGS and FCFLAGS applied to a Debian build (you can see where it's applied at configure time in the build log e.g. at https://buildd.debian.org/status/fetch.php?pkg=petsc&arch=amd64&ver=3.8.3%2Bdfsg1-5&stamp=1520201294&raw=0 )

    The common CFLAGS are generated using dpkg-buildflags. The idea of adding -fdebug-prefix-map is to help achieve reproducible builds, discussed at https://lists.debian.org/debian-devel/2016/07/msg00373.html and https://wiki.debian.org/ReproducibleBuilds/BuildPathProposal
    It's used as -fdebug-prefix-map=/<<BUILDDIR>>/petsc-3.8.3+dfsg1=. i.e. the idea of the flag is to replace references to the builddir with a reference to "." instead.

    These are configure time flags, so that's why petscconfiginfo.h, petscmachineinfo.h and reconfigure-x86_64-linux-gnu-real.py have a record of them. I can scrub the builddir references here, the same way that our build log does (converting to <<BUILDDIR>>)

    petscvariables also keeps a record of configure flags in CONFIGURE_OPTIONS. How important is it to remain there?

    I'm less certain about placing these configure flags in cflags_extra in PETSc.pc. The configure flags are used to build PETSc, but the pkgconfig flags in PETSc.pc should be for building with PETSc, not for building PETSc. Does the way cflags_extra and fflags_extra are being used in PETSc.pc make sense?

    2 . thanks for fixing testfiles

    3 . Good point for discussion. Is source needed there in uninstall.py? At the end of the day, package manager systems like debian don't need uninstall.py. I can remove it from our package if that's a better option.

    4,5. All your other points make sense. They're not a problem for the builddir issue we're looking at here.

  6. Jed Brown
    1. So cflags_extra is not seen by normal users of pkg-config. It needs to be specifically referenced pkg-config --variable=cflags_extra PETSc. We provide those flags so that users who want to use the same flags as were used to build PETSc can get them (especially important for -m32 and similar flags that may change the ABI, but also convenient for debug/opt and compilers that use weird options naming schemes like IBM XL). If you want this particular flag removed by PETSc's configure, it would be by blacklisting it or by creating a new set of variables for options that PETSc uses to build and then hides from its build products.

    2. @BarryFSmith @Satish Balay Why does the uninstall script contain (src, dst) pairs instead of just the installed files? The code does not reference src:

    for src, dst in copies:
      if os.path.exists(dst):
        os.remove(dst)
    

    (It should also remove any empty directories that it leaves behind.)

  7. Satish Balay

    9826f787497d486a5638530a689d66bc5e109433

    wrt 'uninstall script contain (src, dst)' - looks like thats how the code was originally written.

    Wrt removing empty dirs - there was commit that deleted more than it should - that I reverted. It need a proper fix
    [i.e have a list of all dirs for the files that are installed - and go through this list (in the correct order) and delete only if the dir is empty]

  8. Satish Balay

    commit 96f2612dcd2cee4316d1cbe8feb15bde1571a25e

    uninstall: remove build path listing for installed files from uninstall script
    Also attept to delete dirs that were crated by the installer
    

    The following still needs fixing [they are not captured by the uninstall script?]

    • the link lib/libpetsc.so.3.08@
    • the testfiles installed in prefix/share/petsc/examples

    And should the uninstall script delete itself? [and the prefixdir?]

  9. Drew Parsons reporter

    @Jed Brown thanks for the explanation about cflags_extra. I agree, it could be useful to have the exact configuration environment at hand if users want to build their PETSc clients under matching conditions.

    Since it's Debian that's imposing the builddir reference there in -fdebug-prefix-map, it'll make best sense for me to deal with that within the Debian build system.

    Thanks Satish for the patches.

  10. Satish Balay
    1. is -fdebug-prefix-map primarily required for build - and not for configure? If so - one could do:
      .- /configure [without -fdebug-prefix-map]
      - make all CFLAGS=-fdebug-prefix-map

    2. Wrt " At the end of the day, package manager systems like debian don't need uninstall.py" - yeah - since package managers usually provide uninstall functionality [ except apple package manager :/ ] uninstall.py is of no use here - and probably should not be packaged.

    Its primarily useful for users installing petsc from source. Also - its not clear if anyone actually uses it.. I myself don't trust [other] packages to provide 'make uninstall' - and rely on installing in a way so that I can use 'rm -rf prefix-dir' for unstalling that package.

  11. Satish Balay

    Hm - perhaps the following change? I have to verify

    balay@asterix /home/balay/petsc (balay/prefix-uninstall *=)
    $ git diff |cat
    diff --git a/config/PETSc/Configure.py b/config/PETSc/Configure.py
    index 25c4deeb80..9bce221b32 100644
    --- a/config/PETSc/Configure.py
    +++ b/config/PETSc/Configure.py
    @@ -413,7 +413,6 @@ prepend-path PATH "%s"
           else:
             self.addMakeMacro('PETSC_FC_INCLUDES',self.headers.toStringNoDupes(includes))
    
    -    self.addMakeMacro('DESTDIR',self.installdir.dir)
         self.addDefine('LIB_DIR','"'+os.path.join(self.installdir.dir,'lib')+'"')
    
         if self.framework.argDB['with-single-library']:
    diff --git a/config/install.py b/config/install.py
    index 453b50ff90..eb87f87cd1 100755
    --- a/config/install.py
    +++ b/config/install.py
    @@ -63,8 +63,8 @@ class Installer(script.Script):
    
       def setupDirectories(self):
         self.rootDir    = self.petscdir.dir
    -    self.destDir    = os.path.abspath(self.argDB['destDir'])
         self.installDir = os.path.abspath(os.path.expanduser(self.framework.argDB['prefix']))
    +    self.destDir    = os.path.abspath(self.argDB['destDir']+self.installDir)
         self.arch       = self.arch.arch
         self.archDir           = os.path.join(self.rootDir, self.arch)
         self.rootIncludeDir    = os.path.join(self.rootDir, 'include')
    

    Also checkDestdir() can perhaps be tossed out

  12. Satish Balay

    from the reference $(DESTDIR)$(prefix) it wasn't clear if it was equivalent to $(DESTDIR)/$(prefix) - so I used string concat instead of os.path.join().

    My simple test went through [and I think we can keep checkDestdir().

    If ok - I can add this change [with the os.path.join() fix] to balay/prefix-uninstall - and merge to next

  13. Drew Parsons reporter

    For the most part, adding to CFLAGS is the right behaviour for these debian CFLAGS. Apart from -fdebug-prefix-map, they add options for hardening, specifically, -g -O2 -fstack-protector-strong -Wformat -Werror=format-security.

    Actually -g -O2 could make some trouble relative to configure options. But I don't think it will make too much drama in practice.

  14. Drew Parsons reporter

    For os.path, stripping root could be an option, os.path.join(DESTDIR, prefix.lstrip(os.sep)). Though it might cause problems on a Windows build due to the drive C:/ prefix.

  15. Satish Balay

    wrt CFLAGS - I think its best to list only the flags that you were planing on weeding out with make - and all other options with configure. i.e

    configure CFLAGS='-g -O2 -fstack-protector-strong -Wformat -Werror=format-security' COPTFLAGS=''

    make CFLAGS=-fdebug-prefix-map

  16. Satish Balay

    wrt $(DESTDIR)$(prefix) - I'm still inclined to use string concat

    + self.destDir = os.path.abspath(self.argDB['destDir']+self.installDir)

    For one - os.path.join(DESTDIR, prefix.lstrip(os.sep)) - is using a string op anyway. But primarily - the $(DESTDIR)$(prefix) notation assumes the values are:

    • make install DESTDIR='' prefix='/usr/local'

    • make install DESTDIR='/tmp' prefix=/usr/local'

    But with os.path - I whould have to change the first one [either at api or internally] to:

    • make install DESTDIR='/' prefix='/usr/local'

    Wrt windows - we use cygwin tools - with cygwin PATH notation for the tools [so this part of the code should be fine.

  17. Drew Parsons reporter

    balay/prefix-uninstall (with the commits for pull 897) builds fine for me, handles DESTDIR as expected.

    lib/petsc/conf/testfiles is still getting installed.

    There's one reference to builddir left in lib/petsc/conf/petscvariables, in the DIR variable.

    There's also a reference to builddir in lib/petsc/conf/RDict.db, in an entry that also contains \'_Logger__root\'. RDict.db also contains the builddir path to .../lib/petsc/bin/win32fe

  18. Satish Balay

    pushed fixes for all 3

    • do not install 'testfiles'

    • remove DIR fro petscvariables - it doesn't appear to be used anywhere

    • do not install RDict.db - I don't think its needed after install

  19. Log in to comment