Opened 3 years ago
Closed 2 years ago
#29082 closed defect (fixed)
Do not stage .pc files in src/lib/, clean old generated *.pc files at 'make distclean', fix 'permission denied' errors
Reported by:  dimpase  Owned by:  

Priority:  blocker  Milestone:  sage9.1 
Component:  build: configure  Keywords:  
Cc:  embray, mkoeppe, jhpalmieri, vbraun, fbissey  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  329843b (Commits, GitHub, GitLab)  Commit:  329843baa9cecfb751e8afdb6b666f01930f0bf0 
Dependencies:  Stopgaps: 
Description (last modified by )
#29003 introduced 2stage 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 readonly files:
[openblas0.3.6.p0] Wrote /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/var/tmp/sage/build/openblas0.3.6.p0/inst/Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/lib/pkgconfig/blas.pc [openblas0.3.6.p0] Wrote /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/var/tmp/sage/build/openblas0.3.6.p0/inst/Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/lib/pkgconfig/cblas.pc [openblas0.3.6.p0] Wrote /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/var/tmp/sage/build/openblas0.3.6.p0/inst/Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/lib/pkgconfig/lapack.pc [openblas0.3.6.p0] [openblas0.3.6.p0] real 8m40.259s [openblas0.3.6.p0] user 45m2.739s [openblas0.3.6.p0] sys 5m48.754s [openblas0.3.6.p0] Copying package files from temporary location /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/var/tmp/sage/build/openblas0.3.6.p0/inst to /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local [openblas0.3.6.p0] cp: /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/./lib/pkgconfig/cblas.pc: Permission denied [openblas0.3.6.p0] cp: /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/./lib/pkgconfig/blas.pc: Permission denied [openblas0.3.6.p0] cp: /Users/mkoeppe/s/sage/sagerebasing/worktreealgebraic2018spring/.tox/localhomebrewminimal/local/./lib/pkgconfig/lapack.pc: Permission denied
This ticket revises the installation as follows:
configure
no longer creates .pc files in the build tree Instead it creates rules in
make/Makefile
that create the .pc files inSAGE_LOCAL
.  Before pc files are installed into
SAGE_LOCAL
, the targets are removed, to avoid permission problems
This ticket does not solve everything. There may still be problems when switching from a system library to an spkgprovided library. We will use the followup ticket #29387 (Complete solution for installing the generated *.pc files) to address this problem.
Change History (70)
comment:1 Changed 3 years ago by
 Cc mkoeppe added
 Status changed from new to needs_info
comment:2 followup: ↓ 7 Changed 3 years ago by
comment:3 Changed 3 years ago by
 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 3 years ago by
 Status changed from needs_info to needs_work
comment:5 followup: ↓ 6 Changed 3 years ago by
(deleted a comment that was intended for #29071.)
comment:6 in reply to: ↑ 5 Changed 3 years ago by
(deleted a comment that was intended for #29071.)
comment:7 in reply to: ↑ 2 Changed 3 years ago by
 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 intosrc/
.
Perhaps we should just create var/
next to src/
and put these things there?
comment:8 Changed 3 years ago by
No, I don't think so. These are configuregenerated files of sagethedistribution  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 3 years ago by
 Dependencies changed from #29071 to #29051, #29071, #29084
comment:10 Changed 3 years ago by
comment:11 Changed 3 years ago by
 Branch set to u/mkoeppe/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_
comment:12 Changed 3 years ago by
 Commit set to 2a01db11ef106d00217d3176a794850524467bcd
 Description modified (diff)
Last 10 new commits:
683a926  make installing *.pc files conditional

a0acbcb  correct quoting of m4 index variable

5ee6844  ensure gfortran is available

56541ae  wrap AC_FC_FUNC so that it does not throw an error without Fortran

9d1770a  revert #29025, remove useless [ALL] section

dba7aef  revert from "libraries=" back to sectionspecific "BLAH_libs="

3a4524e  Merge remotetracking branch 'trac/public/packages/numpy/no_DEFAULT_and_ALL_numpy_site_cfg' into openblaspcfix

06f46eb  Add blas to fflas_ffpack linked libraries so that openblas is picked up on Arch

5f16988  Merge 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_

2a01db1  Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade

comment:13 Changed 3 years ago by
comment:14 Changed 3 years ago by
comment:15 Changed 3 years ago by
 Dependencies changed from #29051, #29071, #29084 to #29051, #29071, #29084, #26130, #29056
comment:16 Changed 3 years ago by
 Priority changed from blocker to major
I will redo this on top of the hot fix  #29121.
comment:17 Changed 2 years ago by
 Description modified (diff)
comment:18 Changed 2 years ago by
 Commit changed from 2a01db11ef106d00217d3176a794850524467bcd to 2535c186176468a691627bdc0cff84d8387782a8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2535c18  Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade

comment:19 Changed 2 years ago by
 Dependencies #29051, #29071, #29084, #26130, #29056 deleted
 Status changed from needs_work to needs_review
Rebased on 9.1.beta4
comment:20 Changed 2 years ago by
 Cc jhpalmieri added
comment:21 Changed 2 years ago by
 Commit changed from 2535c186176468a691627bdc0cff84d8387782a8 to 3f2060ebf71d3cd593efe44a751b1a8017af969b
Branch pushed to git repo; I updated commit sha1. New commits:
3f2060e  Merge tag '9.1.beta5' into t/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_

comment:23 Changed 2 years ago by
 Commit changed from 3f2060ebf71d3cd593efe44a751b1a8017af969b to d6b5677f0a51dd790377482f4608fd041fb0fbbe
Branch pushed to git repo; I updated commit sha1. New commits:
d6b5677  Merge tag '9.1.beta6' into t/29082/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_

comment:24 Changed 2 years ago by
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 "specialpurpose code". It's a quite generic solution that you seem to be replacing with "specialpurpose code" that's very specific to openblas. This ticket was needed for more than just blas.
comment:25 Changed 2 years ago by
 Status changed from needs_review to needs_info
comment:26 Changed 2 years ago by
 Description modified (diff)
comment:27 Changed 2 years ago by
Is my ticket handling the gsl.pc
correctly? Which packages use this file?
comment:28 Changed 2 years ago by
 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 2 years ago by
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 2 years ago by
It seemed to be an elegant solution when I wrote this a month ago
comment:31 Changed 2 years ago by
It seems that your package handles more that blas/lapack pc files, no? Perhaps it just is wrongly named?
comment:32 Changed 2 years ago by
That's basically my question regarding gsl.pc above, I guess.
comment:33 Changed 2 years ago by
there is also a bug: PKG_CONFIG_PATH is pointing to the wrong directory, so these newly installed pc files are not found:
(sagesh) dima@fedora:sagetracmirror$ echo $PKG_CONFIG_PATH /home/dima/sagetracmirror/local/lib/pkgconfig:. (sagesh) dima@fedora:sagetracmirror$ find . name cblas.pc ./build/pkgs/sage_system_blas_facade/src/cblas.pc ./src/lib/pkgconfig/cblas.pc
comment:34 Changed 2 years ago by
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 2 years ago by
Feel free to make changes  otherwise I can look into it tomorrow
comment:36 Changed 2 years ago by
What does build/pkgs/sage_system_blas_facade/spkginstall
look like on your machine?
comment:37 Changed 2 years ago by
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 2 years ago by
Looks like some spaces are missing
comment:39 Changed 2 years ago by
indeed, one needs

build/pkgs/openblas/spkgconfigure.m4
a b SAGE_SPKG_CONFIGURE([openblas], [dnl CHECK 37 37 m4_foreach([blaslibnam], [blas, cblas, lapack], [ 38 38 AS_IF([test x$sage_install_]blaslibnam[_pc = xyes], [ 39 39 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]' '])]) 41 41 AC_SUBST([SAGE_BLAS], [sage_system_blas_facade]) 42 42 ]) 43 43 ])
comment:40 Changed 2 years ago by
but this does not make any difference. As far as I can see, spkginstall
of sage_system_blas_facade
never gets run  I'm lost in untangling the reason for this.
comment:41 Changed 2 years ago by
OK, I'll take care of it later today.
comment:42 Changed 2 years ago by
 Status changed from needs_info to needs_work
comment:43 Changed 2 years ago by
 Dependencies set to #29287
comment:44 Changed 2 years ago by
 Commit changed from d6b5677f0a51dd790377482f4608fd041fb0fbbe to 30d3b8245cbcee8c7b629dfebc9de213e89f7c86
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
d1fd7a7  src/bin/sagelistpackages: Remove use of type=pip

75175db  sage.misc.package.list_packages: For unversioned packages, use 'none' as version

d3d78f4  m4/sage_spkg_collect.m4: Fix up collection of dependencies

dbb9860  build/pkgs/texlive/spkginstall: Fix up path

8b1d5ca  trac 29287: rewording documentation

057a66b  Make pyopenssl a pip package

8f6323e  src/sage/env.py: Fix up doctest on starting sage without SAGE_* variables

0919086  trac 29287: doc fixes

6781790  Move all BLAS PC file installation logic to new script package sage_sage_system_blas_facade

30d3b82  build/pkgs/openblas/spkgconfigure.m4: Separate library names by spaces

comment:45 Changed 2 years ago by
Branch is on top of #29287
comment:46 Changed 2 years ago by
 Commit changed from 30d3b8245cbcee8c7b629dfebc9de213e89f7c86 to 7fd5e31fa51e0f9a662848450dbae37732468d6f
comment:47 Changed 2 years ago by
 Commit changed from 7fd5e31fa51e0f9a662848450dbae37732468d6f to b493a32df5c9bb08613e370759412e55a2501676
Branch pushed to git repo; I updated commit sha1. New commits:
b493a32  install sage_system_blas_facade

comment:48 Changed 2 years ago by
 Priority changed from major to blocker
this prevents building on Fedora after distclean
comment:49 Changed 2 years ago by
I'm working on a simpler version of this ticket.
comment:50 Changed 2 years ago by
 Description modified (diff)
comment:51 Changed 2 years ago by
 Dependencies #29287 deleted
comment:52 Changed 2 years ago by
 Commit changed from b493a32df5c9bb08613e370759412e55a2501676 to 705da6a06eaa69613f0c25149f7e9e2d21833371
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
705da6a  Link/generate pc files in make, not configure

comment:53 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:54 Changed 2 years ago by
 Commit changed from 705da6a06eaa69613f0c25149f7e9e2d21833371 to d86cd33c67e253dd17992a8a470a6391663892e2
Branch pushed to git repo; I updated commit sha1. New commits:
d86cd33  fixup

comment:55 Changed 2 years ago by
 Commit changed from d86cd33c67e253dd17992a8a470a6391663892e2 to f5b4f625d6e3001f07123974b4d1f5356545ae03
Branch pushed to git repo; I updated commit sha1. New commits:
f5b4f62  Makefile [miscclean]: No need to remove build/lib/pkgconfig

comment:56 Changed 2 years ago by
 Status changed from needs_review to needs_work
Error with gsl.pc for localhomebrewmacosstandard
 https://github.com/mkoeppe/sage/runs/524361197
comment:57 Changed 2 years ago by
also ubuntubionicstandard
(sagelib and cvxopt)
comment:58 Changed 2 years ago by
 Commit changed from f5b4f625d6e3001f07123974b4d1f5356545ae03 to 2a38a167ab0c6aa3f39d704eb4087d9adf36c85a
Branch pushed to git repo; I updated commit sha1. New commits:
2a38a16  build/pkgs/gsl/spkgconfigure.m4: Quoting fix

comment:59 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:60 Changed 2 years ago by
comment:61 Changed 2 years ago by
 Description modified (diff)
comment:62 Changed 2 years ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
OK, this does the needed job. Perhaps the make rules could be made less explicit, but OK.
comment:63 Changed 2 years ago by
Thanks!
comment:64 Changed 2 years ago by
 Summary changed from Move .pc file from src/ to build/, clean generated *.pc files at 'make distclean', fix 'permission denied' errors to Do not stage .pc files in src/lib/, clean old generated *.pc files at 'make distclean', fix 'permission denied' errors
comment:65 Changed 2 years ago by
 Status changed from positive_review to needs_work
Make distclean deletes src/lib/pkgconfig/.gitignore
comment:66 Changed 2 years ago by
 Commit changed from 2a38a167ab0c6aa3f39d704eb4087d9adf36c85a to 329843baa9cecfb751e8afdb6b666f01930f0bf0
Branch pushed to git repo; I updated commit sha1. New commits:
329843b  Ignore /src/lib/pkgconfig using toplevel .gitignore

comment:67 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:68 Changed 2 years ago by
 Status changed from needs_review to positive_review
ok. sorry for overlooking this.
comment:69 Changed 2 years ago by
Thanks
comment:70 Changed 2 years ago by
 Branch changed from u/mkoeppe/move__pc_file_from_src__to_build___clean_generated___pc_files_at__make_distclean_ to 329843baa9cecfb751e8afdb6b666f01930f0bf0
 Resolution set to fixed
 Status changed from positive_review to closed
My suggestion would be to rewrite this as a script package instead of putting things into
src/
. This stuff really does not belong intosrc/
.