#28349 closed defect (duplicate)

Problem with SAGE_INC in module_list.py when using system packages

Reported by: embray Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: build Keywords:
Cc: dimpase Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: u/embray/ticket-28349 (Commits, GitHub, GitLab) Commit: c775407d82a013e9c3b9ac72fe2f73f0d3d96527
Dependencies: Stopgaps:

Status badges

Description (last modified by embray)

I noticed in a recent build of Sage on a Debian buster VM, using many system packages, a problem:

Every time I try to re-build, whether I run make build, sage -b, etc. two Python modules get recompiled (from their Cython-generated C sources, though Cython itself is not being re-run). Specifically,

src/build/cythonized/sage/modules/vector_mod2_dense.c
src/build/cythonized/sage/matrix/matrix_mod2_dense.c

One of the things that these two modules have in common, and unique from other modules (as listed in src/module_list.py) is their dependency on SAGE_INC + '/png.h'. But since I'm using the system libpng, this file does not exist, so those modules constantly get rebuilt.

I'm not really sure the best thing to do about this right now. Maybe we should only add those headers to the depends list if they actually exist (and just assume that system headers won't be updated often...)

Change History (21)

comment:1 Changed 20 months ago by embray

  • Description modified (diff)

comment:2 Changed 20 months ago by dimpase

good catch. I noticed this too, and was wondering what it is...

comment:3 follow-up: Changed 20 months ago by jdemeyer

This file doesn't actually depend on png.h in any way that I can see, so we should just drop that dependency (regardless of whether it fixes the issue on this ticket).

comment:4 Changed 20 months ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:5 Changed 20 months ago by embray

  • Authors changed from Jeroen Demeyer to Erik Bray
  • Branch set to u/embray/ticket-28349
  • Commit set to c775407d82a013e9c3b9ac72fe2f73f0d3d96527

New commits:

ec2a458Trac #28349: Move include files from Extension.depends into kwds
c775407Trac #28349: Don't explicit use SAGE_INC for most header paths

comment:6 in reply to: ↑ 3 ; follow-up: Changed 20 months ago by embray

Replying to jdemeyer:

This file doesn't actually depend on png.h in any way that I can see, so we should just drop that dependency (regardless of whether it fixes the issue on this ticket).

It does depend indirectly through gd.h, which it does depend on directly. But I agree it's a bit odd to include png.h in that list explicitly.

comment:7 Changed 20 months ago by embray

Sorry--pressed submit without seeing you had added to this. Maybe you were going to change something?

Nevertheless, the problem is still more general than png.h. It would be better not to use SAGE_INC in this way (e.g. the same problem applies for m4ri, I just don't have a system package for that yet).

comment:8 follow-up: Changed 20 months ago by dimpase

+from sage.env import SAGE_LOCAL, SAGE_INC

would surely be inconvenient for distributions that don't use sage.env.

comment:9 Changed 20 months ago by jdemeyer

  • Branch changed from u/embray/ticket-28349 to u/jdemeyer/ticket-28349

comment:10 Changed 20 months ago by embray

  • Commit changed from c775407d82a013e9c3b9ac72fe2f73f0d3d96527 to 7100fa7c4e530c2e56656dc7f19541b2882954ab

Apparently in the depends = ['png.h] traces back to #5265, and didn't really make sense then either. When vector_mod2_dense was added it was just carried over as cargo cult.


New commits:

7100fa7Fix dependencies on png.h and m4ri.h

comment:11 Changed 20 months ago by jdemeyer

  • Branch changed from u/jdemeyer/ticket-28349 to u/embray/ticket-28349
  • Commit changed from 7100fa7c4e530c2e56656dc7f19541b2882954ab to c775407d82a013e9c3b9ac72fe2f73f0d3d96527

New commits:

ec2a458Trac #28349: Move include files from Extension.depends into kwds
c775407Trac #28349: Don't explicit use SAGE_INC for most header paths

comment:12 Changed 20 months ago by embray

Heh, perhaps we can merge these two fixes? I think Jeroen's branch makes sense in its own right.

comment:13 Changed 20 months ago by jdemeyer

I created #28352 with my fix.

comment:14 in reply to: ↑ 6 Changed 20 months ago by jdemeyer

Replying to embray:

It does depend indirectly through gd.h, which it does depend on directly.

My gd.h does not depend on png.h

comment:15 Changed 20 months ago by jdemeyer

The gd library does depend on the png library, but that's independent of the header files.

comment:16 in reply to: ↑ 8 Changed 20 months ago by fbissey

Replying to dimpase:

+from sage.env import SAGE_LOCAL, SAGE_INC

would surely be inconvenient for distributions that don't use sage.env.

Some of us are phasing out sage-env but we do love sage.env.

comment:17 Changed 16 months ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:18 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

If I'm not mistaken, this was fixed by #28352?

comment:19 Changed 12 months ago by dimpase

this one has been fixed, but similar problems pop up elsewhere, e.g. I saw similar parasite rebuilds with the branch of #29369 (currently disabled) - apparently

    Extension('sage.rings.polynomial.pbori',
              sources = ['sage/rings/polynomial/pbori.pyx'],
              libraries=['brial', 'brial_groebner'] + m4ri_libs + png_libs,
              library_dirs = m4ri_library_dirs + png_library_dirs,
              include_dirs = m4ri_include_dirs + png_include_dirs,
              depends = [SAGE_INC + "/polybori/" + hd + ".h" for hd in ["polybori", "config"]],
              extra_compile_args = m4ri_extra_compile_args),

is to blame

comment:20 Changed 12 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:21 Changed 11 months ago by chapoton

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.