Opened 23 months ago

Last modified 21 months ago

#29082 closed defect

Move .pc file from src/ to build/, clean generated *.pc files at 'make distclean', fix 'permission denied' errors — at Version 50

Reported by: dimpase Owned by:
Priority: blocker Milestone: sage-9.1
Component: build: configure Keywords:
Cc: embray, mkoeppe, jhpalmieri, vbraun, fbissey Merged in:
Authors: Matthias Koeppe Reviewers:
Report Upstream: N/A Work issues:
Branch: u/mkoeppe/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_ (Commits, GitHub, GitLab) Commit: b493a32df5c9bb08613e370759412e55a2501676
Dependencies: #29287 Stopgaps:

Status badges

Description (last modified by mkoeppe)

#29003 introduced 2-stage installation of generated *.pc files, via src/lib/pkgconfig- but did not provide a way to clean them up at all. And this is a problem (cf. e.g. #29071). Also, in some installations, repeated installations this code leads to 'permission denied' errors when trying to overwrite read-only files:

  [openblas-0.3.6.p0]   Wrote /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/var/tmp/sage/build/openblas-0.3.6.p0/inst/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/lib/pkgconfig/blas.pc
  [openblas-0.3.6.p0]   Wrote /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/var/tmp/sage/build/openblas-0.3.6.p0/inst/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/lib/pkgconfig/cblas.pc
  [openblas-0.3.6.p0]   Wrote /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/var/tmp/sage/build/openblas-0.3.6.p0/inst/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/lib/pkgconfig/lapack.pc
  [openblas-0.3.6.p0]   
  [openblas-0.3.6.p0]   real	8m40.259s
  [openblas-0.3.6.p0]   user	45m2.739s
  [openblas-0.3.6.p0]   sys	5m48.754s
  [openblas-0.3.6.p0]   Copying package files from temporary location /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/var/tmp/sage/build/openblas-0.3.6.p0/inst to /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local
  [openblas-0.3.6.p0]   cp: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/./lib/pkgconfig/cblas.pc: Permission denied
  [openblas-0.3.6.p0]   cp: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/./lib/pkgconfig/blas.pc: Permission denied
  [openblas-0.3.6.p0]   cp: /Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/.tox/local-homebrew-minimal/local/./lib/pkgconfig/lapack.pc: Permission denied

This ticket revises the 2-stage installation as follows:

  • Because the generated files are for sage-the-distribution, not sagelib, they belong into build, not src.
  • Because configure (config.status) creates them, make distclean cleans them.
  • The pc files installed into SAGE_LOCAL are uninstalled when necessary - or it is ensured that they are properly overwritten.
  • Code in build/make/Makefile.in is removed in favor of creating a new type=script package sage_system_blas_facade, whose spkg-install does the installation to SAGE_LOCAL.
  • openblas/spkg-configure.m4 sets SAGE_BLAS=sage_system_blas_facade if PC files are generated.

Change History (50)

comment:1 Changed 23 months ago by dimpase

  • Cc mkoeppe added
  • Status changed from new to needs_info

comment:2 follow-up: Changed 23 months ago by mkoeppe

My suggestion would be to rewrite this as a script package instead of putting things into src/. This stuff really does not belong into src/.

comment:3 Changed 23 months ago by mkoeppe

  • Dependencies set to #29071
  • Description modified (diff)
  • Summary changed from clean generated *.pc files to Move .pc file from src/ to build/, clean generated *.pc files at 'make distclean'

Added #29071 as a dependency as it touches the same files.

comment:4 Changed 23 months ago by mkoeppe

  • Status changed from needs_info to needs_work

comment:5 follow-up: Changed 23 months ago by mkoeppe

(deleted a comment that was intended for #29071.)

Last edited 23 months ago by mkoeppe (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 23 months ago by mkoeppe

(deleted a comment that was intended for #29071.)

Last edited 23 months ago by mkoeppe (previous) (diff)

comment:7 in reply to: ↑ 2 Changed 23 months ago by dimpase

  • Description modified (diff)

Replying to mkoeppe:

My suggestion would be to rewrite this as a script package instead of putting things into src/. This stuff really does not belong into src/.

Perhaps we should just create var/ next to src/ and put these things there?

comment:8 Changed 23 months ago by mkoeppe

No, I don't think so. These are configure-generated files of sage-the-distribution - like build/make/Makefile. There's no need for a new top level.

Let me take care of this ticket after #29071 is done.

comment:9 Changed 23 months ago by mkoeppe

  • Dependencies changed from #29071 to #29051, #29071, #29084

comment:10 Changed 23 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:11 Changed 23 months ago by mkoeppe

  • Branch set to u/mkoeppe/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_

comment:12 Changed 23 months ago by mkoeppe

  • Commit set to 2a01db11ef106d00217d3176a794850524467bcd
  • Description modified (diff)

Last 10 new commits:

683a926make installing *.pc files conditional
a0acbcbcorrect quoting of m4 index variable
5ee6844ensure gfortran is available
56541aewrap AC_FC_FUNC so that it does not throw an error without Fortran
9d1770arevert #29025, remove useless [ALL] section
dba7aefrevert from "libraries=" back to section-specific "BLAH_libs="
3a4524eMerge remote-tracking branch 'trac/public/packages/numpy/no_DEFAULT_and_ALL_numpy_site_cfg' into openblaspcfix
06f46ebAdd blas to fflas_ffpack linked libraries so that openblas is picked up on Arch
5f16988Merge branch 'u/arojas/make_fflas_ffpack_detect_and_use_system_openblas_on_arch' of git://trac.sagemath.org/sage into t/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_
2a01db1Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade

comment:13 Changed 23 months ago by mkoeppe

Branch is on top of #29051, #29071, #29084. This is a first version, haven't tested much yet.

comment:15 Changed 22 months ago by mkoeppe

  • Dependencies changed from #29051, #29071, #29084 to #29051, #29071, #29084, #26130, #29056

comment:16 Changed 22 months ago by mkoeppe

  • Priority changed from blocker to major

I will redo this on top of the hot fix - #29121.

comment:17 Changed 22 months ago by mkoeppe

  • Description modified (diff)

comment:18 Changed 22 months ago by git

  • Commit changed from 2a01db11ef106d00217d3176a794850524467bcd to 2535c186176468a691627bdc0cff84d8387782a8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2535c18Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade

comment:19 Changed 22 months ago by mkoeppe

  • Dependencies #29051, #29071, #29084, #26130, #29056 deleted
  • Status changed from needs_work to needs_review

Rebased on 9.1.beta4

comment:20 Changed 22 months ago by mkoeppe

  • Cc jhpalmieri added

comment:21 Changed 22 months ago by git

  • Commit changed from 2535c186176468a691627bdc0cff84d8387782a8 to 3f2060ebf71d3cd593efe44a751b1a8017af969b

Branch pushed to git repo; I updated commit sha1. New commits:

3f2060eMerge tag '9.1.beta5' into t/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_

comment:22 Changed 22 months ago by mkoeppe

  • Cc vbraun fbissey added

Needs review.

comment:23 Changed 21 months ago by git

  • Commit changed from 3f2060ebf71d3cd593efe44a751b1a8017af969b to d6b5677f0a51dd790377482f4608fd041fb0fbbe

Branch pushed to git repo; I updated commit sha1. New commits:

d6b5677Merge tag '9.1.beta6' into t/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_

comment:24 Changed 21 months ago by embray

Agreed on the first two points in the description.

Don't agree on sage_system_blas_facade and the changes to Makefile.in. I don't see how it's "special-purpose code". It's a quite generic solution that you seem to be replacing with "special-purpose code" that's very specific to openblas. This ticket was needed for more than just blas.

comment:25 Changed 21 months ago by embray

  • Status changed from needs_review to needs_info

comment:26 Changed 21 months ago by mkoeppe

  • Description modified (diff)

comment:27 Changed 21 months ago by mkoeppe

Is my ticket handling the gsl.pc correctly? Which packages use this file?

comment:28 Changed 21 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Move .pc file from src/ to build/, clean generated *.pc files at 'make distclean' to Move .pc file from src/ to build/, clean generated *.pc files at 'make distclean', fix 'permission denied' errors

comment:29 Changed 21 months ago by dimpase

I also don't quite understand the need to create a special script package, instead of adjusting build/make/Makefile.in.

Is there a technical problem?

comment:30 Changed 21 months ago by mkoeppe

It seemed to be an elegant solution when I wrote this a month ago

comment:31 Changed 21 months ago by dimpase

It seems that your package handles more that blas/lapack pc files, no? Perhaps it just is wrongly named?

comment:32 Changed 21 months ago by mkoeppe

That's basically my question regarding gsl.pc above, I guess.

comment:33 Changed 21 months ago by dimpase

there is also a bug: PKG_CONFIG_PATH is pointing to the wrong directory, so these newly installed pc files are not found:

(sage-sh) dima@fedora:sagetrac-mirror$ echo $PKG_CONFIG_PATH
/home/dima/sagetrac-mirror/local/lib/pkgconfig:.
(sage-sh) dima@fedora:sagetrac-mirror$ find . -name cblas.pc
./build/pkgs/sage_system_blas_facade/src/cblas.pc
./src/lib/pkgconfig/cblas.pc

comment:34 Changed 21 months ago by dimpase

oops, I mean, they are just not installed at all. PKG_CONFIG_PATH is correct, but pc files are not there, or anywhere, except the old location

comment:35 Changed 21 months ago by mkoeppe

Feel free to make changes -- otherwise I can look into it tomorrow

comment:36 Changed 21 months ago by mkoeppe

What does build/pkgs/sage_system_blas_facade/spkg-install look like on your machine?

comment:37 Changed 21 months ago by dimpase

here it is:

#! /usr/bin/env bash
PCFILES="blas.pccblas.pclapack.pc"
SAGE_PKGCONFIG="$SAGE_LOCAL/lib/pkgconfig"
mkdir -p "$SAGE_PKGCONFIG"
if [ -n "$PCFILES" ]; then
   cd build/pkgs/sage_system_blas_facade/src && cp -P $PCFILES "$SAGE_PKGCONFIG"
fi

also note that the original openblas.pc is in SAGE_ROOT

comment:38 Changed 21 months ago by mkoeppe

Looks like some spaces are missing

comment:39 Changed 21 months ago by dimpase

indeed, one needs

  • build/pkgs/openblas/spkg-configure.m4

    a b SAGE_SPKG_CONFIGURE([openblas], [dnl CHECK 
    3737       m4_foreach([blaslibnam], [blas, cblas, lapack], [
    3838        AS_IF([test x$sage_install_]blaslibnam[_pc = xyes], [
    3939         AC_CONFIG_LINKS([build/pkgs/sage_system_blas_facade/src/]blaslibnam[.pc:$OPENBLASPCDIR/openblas.pc])
    40          AS_VAR_APPEND([SAGE_SYSTEM_BLAS_FACADE_PC_FILES], blaslibnam[.pc])])
     40         AS_VAR_APPEND([SAGE_SYSTEM_BLAS_FACADE_PC_FILES], [blaslibnam[.pc]' '])])
    4141         AC_SUBST([SAGE_BLAS], [sage_system_blas_facade])
    4242       ])
    4343    ])

comment:40 Changed 21 months ago by dimpase

but this does not make any difference. As far as I can see, spkg-install of sage_system_blas_facade never gets run - I'm lost in untangling the reason for this.

comment:41 Changed 21 months ago by mkoeppe

OK, I'll take care of it later today.

comment:42 Changed 21 months ago by dimpase

  • Status changed from needs_info to needs_work

comment:43 Changed 21 months ago by mkoeppe

  • Dependencies set to #29287

comment:44 Changed 21 months ago by git

  • Commit changed from d6b5677f0a51dd790377482f4608fd041fb0fbbe to 30d3b8245cbcee8c7b629dfebc9de213e89f7c86

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

d1fd7a7src/bin/sage-list-packages: Remove use of type=pip
75175dbsage.misc.package.list_packages: For unversioned packages, use 'none' as version
d3d78f4m4/sage_spkg_collect.m4: Fix up collection of dependencies
dbb9860build/pkgs/texlive/spkg-install: Fix up path
8b1d5catrac 29287: rewording documentation
057a66bMake pyopenssl a pip package
8f6323esrc/sage/env.py: Fix up doctest on starting sage without SAGE_* variables
0919086trac 29287: doc fixes
6781790Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade
30d3b82build/pkgs/openblas/spkg-configure.m4: Separate library names by spaces

comment:45 Changed 21 months ago by mkoeppe

Branch is on top of #29287

comment:46 Changed 21 months ago by git

  • Commit changed from 30d3b8245cbcee8c7b629dfebc9de213e89f7c86 to 7fd5e31fa51e0f9a662848450dbae37732468d6f

Branch pushed to git repo; I updated commit sha1. New commits:

b68595eChange type
7fd5e31fixup

comment:47 Changed 21 months ago by git

  • Commit changed from 7fd5e31fa51e0f9a662848450dbae37732468d6f to b493a32df5c9bb08613e370759412e55a2501676

Branch pushed to git repo; I updated commit sha1. New commits:

b493a32install sage_system_blas_facade

comment:48 Changed 21 months ago by dimpase

  • Priority changed from major to blocker

this prevents building on Fedora after distclean

comment:49 Changed 21 months ago by mkoeppe

I'm working on a simpler version of this ticket.

comment:50 Changed 21 months ago by mkoeppe

  • Description modified (diff)
Note: See TracTickets for help on using tickets.