p_group_cohomology missing dependencies #27173

That's actually good news. When you wrote "API change broke the spkg", I thought I need to change code again (by the way, I am not sure: What is the current official spkg version in Sage? v3.1?).

(an analogous problem occurred with giacpy_sage on #27021). There is no automatic way to do this, but because of the missing dependency on cython (which is what I grepped for) I missed this.

The current dependencies are

singular meataxe | matplotlib gap xz $(SAGERUNTIME)

Really the package does depend on sagelib. Namely, it cimports various Sage types (including Element, Parent).

In an ideal world, it would be possible to have a dependency list

singular meataxe sage.structure.element | matplotlib gap xz $(SAGERUNTIME)

simon-king-jena · 2019-01-30T08:39:50Z

comment:5

I hate the trac interface. Writing my comment destroyed your branch.

Can you provide the branch again?

fchapoton · 2019-01-30T08:41:28Z

New commits:

`eb9a9ff`	`Add missing dependencies of p_group_cohomology`

fchapoton · 2019-01-30T08:41:28Z

Commit: eb9a9ff

fchapoton · 2019-01-30T08:41:28Z

Branch: u/jdemeyer/p_group_cohomology_depends_on_cython

jdemeyer · 2019-01-30T08:46:34Z

comment:7

Replying to @simon-king-jena:

Replying to @jdemeyer:

The fix is simple: rebuilding p_group_cohomology is sufficient

That's actually good news. When you wrote "API change broke the spkg", I thought I need to change code again

No, what really happened is an incompatible change in element.pxd which means that every Cython file cimporting that must be recompiled.

jdemeyer · 2019-01-30T08:46:53Z

comment:8

Replying to @simon-king-jena:

In an ideal world, it would be possible to have a dependency list
singular meataxe sage.structure.element | matplotlib gap xz $(SAGERUNTIME)

Actually, why don't we just do that?

simon-king-jena · 2019-01-30T08:48:53Z

comment:9

Replying to @jdemeyer:

Replying to @simon-king-jena:
In an ideal world, it would be possible to have a dependency list
singular meataxe sage.structure.element | matplotlib gap xz $(SAGERUNTIME)
Actually, why don't we just do that?

Well, when I wrote my comment, I thought that it isn't supported yet.

So, are you saying that a dependency on specific parts of the Sage library is possible? Or would that mean to extend Sage's build system?

sagetrac-git · 2019-01-30T08:52:13Z

Changed commit from eb9a9ff to 0b3ce61

sagetrac-git · 2019-01-30T08:52:13Z

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

`0b3ce61`	`Add missing dependencies of p_group_cohomology`

jdemeyer · 2019-01-30T08:53:24Z

comment:12

Note that this won't pick up transitive dependencies (the Makefile doesn't know that element.pxd depends on sage_object.pxd for example), but at least this should work when changing the file element.pxd.

jdemeyer · 2019-01-30T08:54:29Z

comment:13

Replying to @simon-king-jena:

So, are you saying that a dependency on specific parts of the Sage library is possible?

It's a Makefile, so it is possible to depend on specific filenames. It just never occurred to me to actually do that.

simon-king-jena · 2019-01-30T08:57:57Z

comment:15

Replying to @jdemeyer:

Replying to @simon-king-jena:

So, are you saying that a dependency on specific parts of the Sage library is possible?

It's a Makefile, so it is possible to depend on specific filenames. It just never occurred to me to actually do that.

Nice! I thought the dependencies file may only contain package names.

Also good that you add pip - indeed spkg-install uses pip.

But how to test it? Pull the branch, build sage, touch element.pxd, do make and see if the spkg gets rebuilt?

simon-king-jena · 2019-01-30T09:02:45Z

comment:16

I hate the trac interface.

When I write a comment, I certainly don't want that trac changes the ticket description back to what it was when I started to write my comment!

jdemeyer · 2019-01-30T09:13:05Z

comment:18

Replying to @simon-king-jena:

But how to test it? Pull the branch, build sage, touch element.pxd, do make and see if the spkg gets rebuilt?

Yes, something like that.

simon-king-jena · 2019-01-30T09:22:08Z

comment:19

I think it would be possible to be a little more thorough. Here is the list of all cimports in the spkg that get stuff from outside the spkg (hence, that's dependencies):

src/pGroupCohomology/cochain.pxd:from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
src/pGroupCohomology/cochain.pxd:from sage.libs.meataxe cimport *
src/pGroupCohomology/cochain.pxd:from sage.structure.element cimport RingElement, ModuleElement, Element
src/pGroupCohomology/cochain.pxd:from sage.rings.morphism cimport RingHomomorphism
src/pGroupCohomology/cochain.pyx:from sage.structure.element cimport RingElement
src/pGroupCohomology/cochain.pyx:from sage.matrix.matrix_gfpn_dense cimport new_mtx
src/pGroupCohomology/cochain.pyx:from libc.string cimport memcpy
src/pGroupCohomology/cochain.pyx:from cysignals.signals cimport sig_check, sig_on, sig_off
src/pGroupCohomology/cohomology.pyx:from libc.string cimport memcpy
src/pGroupCohomology/cohomology.pyx:from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
src/pGroupCohomology/cohomology.pyx:from sage.matrix.matrix_gfpn_dense cimport new_mtx
src/pGroupCohomology/cohomology.pyx:from sage.libs.meataxe cimport *
src/pGroupCohomology/modular_cohomology.pyx:from sage.libs.meataxe cimport *
src/pGroupCohomology/modular_cohomology.pyx:from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
src/pGroupCohomology/modular_cohomology.pyx:from sage.matrix.matrix_gfpn_dense cimport new_mtx
src/pGroupCohomology/resolution_bindings.pxd:from sage.libs.meataxe cimport *
src/pGroupCohomology/resolution.pxd:from sage.libs.meataxe cimport *
src/pGroupCohomology/resolution.pxd:from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
src/pGroupCohomology/resolution.pyx:from libc.string cimport memcpy
src/pGroupCohomology/resolution.pyx:from cysignals.memory cimport sig_free
src/pGroupCohomology/resolution.pyx:from cysignals.signals cimport sig_check, sig_on, sig_off
src/pGroupCohomology/resolution.pyx:from sage.matrix.matrix_gfpn_dense cimport Matrix_gfpn_dense as MTX
src/pGroupCohomology/resolution.pyx:from sage.matrix.matrix_gfpn_dense cimport FieldConverter_class, FieldConverter, new_mtx
src/pGroupCohomology/resolution.pyx:from sage.matrix.matrix0 cimport Matrix as Matrix0

So, the complete lists of dependencies is

sage.matrix.matrix_gfpn_dense
sage.libs.meataxe
sage.structure.element
sage.rings.morphism
libc.string
cysignals.signals
cysignals.memory
sage.matrix.matrix0

So, I suggest to include all this in the list of dependencies. Which gives rise to the questions:

Has the dependency list be in a single line, or can it be one dependency per line? Would look nicer!
What to do about libc and cysignals? Is that covered by the dependencies python and cython?

slel · 2019-01-30T09:40:26Z

comment:20

Replying to @simon-king-jena:

Must the dependency list be in a single line, or can it be one dependency per line? Would look nicer!

Only the first line of the dependencies file is read, so yes, the dependencies must all
be listed in the first line of that file.

slel · 2019-01-30T09:40:26Z

Changed keywords from none to dependencies

simon-king-jena · 2019-01-30T10:41:50Z

Changed branch from u/jdemeyer/p_group_cohomology_depends_on_cython to u/SimonKing/p_group_cohomology_depends_on_cython

simon-king-jena · 2019-01-30T10:45:47Z

Changed commit from 0b3ce61 to 46b2823

simon-king-jena · 2019-01-30T10:45:47Z

comment:22

I have added all dependencies in sagelib, i.e., all pxd-files from which p_group_cohomology cimports.

So, what's missing is libc and cysignals. How to name them in the dependencies?

New commits:

`46b2823`	`Add further p_group_cohomology dependencies from sagelib`

jdemeyer · 2019-01-30T14:25:05Z

comment:23

Replying to @simon-king-jena:

So, the complete lists of dependencies is

sage.matrix.matrix_gfpn_dense
sage.libs.meataxe
sage.structure.element
sage.rings.morphism
libc.string
cysignals.signals
cysignals.memory
sage.matrix.matrix0

No, that's not the complete list of dependencies. Like I said, you should also consider implied dependencies: for example, sage.structure.element depends on sage.structure.parent and so on. So my choice of sage.structure.element and sage.matrix.matrix_gfpn_dense was meant to be a reasonably small but meaningful set of dependencies.

For cysignals, just add a dependency on the cysignals package.

simon-king-jena · 2019-01-30T16:02:35Z

comment:24

Replying to @jdemeyer:

No, that's not the complete list of dependencies. Like I said, you should also consider implied dependencies:

I understood that. But I think to the very least one should name all pxd files from which the package cimports. Everything else would be too short.

For cysignals, just add a dependency on the cysignals package.

Thanks. And for libc?

sagetrac-git · 2019-01-31T14:18:42Z

Changed commit from 46b2823 to f39c9f1

sagetrac-git · 2019-01-31T14:18:42Z

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

`f39c9f1`	`Add cysignals to p_group_cohomology's dependencies list`

simon-king-jena · 2019-01-31T14:20:00Z

Changed author from Jeroen Demeyer to Jeroen Demeyer, Simon King

simon-king-jena · 2019-01-31T14:20:00Z

comment:26

I have added cysignals to the dependencies. What about libc? In any case, it is still "needs review", and since I added stuff (after searching for all cimports), I guess I qualify as one author.

jdemeyer · 2019-01-31T15:20:45Z

comment:27

Replying to @simon-king-jena:

What about libc?

That's just the system C library on which virtually everything depends. It's assumed to remain ABI-compatible, so recompiling isn't needed.

jdemeyer · 2019-01-31T21:44:11Z

Reviewer: Simon King, Jeroen Demeyer

vbraun · 2019-02-04T23:02:35Z

Changed branch from u/SimonKing/p_group_cohomology_depends_on_cython to f39c9f1

jdemeyer added this to the sage-8.7 milestone Jan 30, 2019

jdemeyer added c: packages: optional labels Jan 30, 2019

jdemeyer changed the title ~~p_group_cohomology depends on Cython~~ p_group_cohomology missing dependencies Jan 30, 2019

jdemeyer added the s: needs review label Jan 30, 2019

jdemeyer added s: needs work and removed s: needs review labels Jan 30, 2019

jdemeyer added s: needs review and removed s: needs work labels Jan 30, 2019

This comment has been minimized.

Sign in to view

jdemeyer added s: positive review and removed s: needs review labels Jan 31, 2019

vbraun removed the s: positive review label Feb 4, 2019

vbraun closed this as completed in 0eba5af Feb 4, 2019

p_group_cohomology missing dependencies #27173

p_group_cohomology missing dependencies #27173

Comments

jdemeyer commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

fchapoton commented Jan 30, 2019

fchapoton commented Jan 30, 2019

fchapoton commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

sagetrac-git mannequin commented Jan 30, 2019

sagetrac-git mannequin commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

This comment has been minimized.

This comment has been minimized.

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

This comment has been minimized.

jdemeyer commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

This comment has been minimized.

slel commented Jan 30, 2019

slel commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

jdemeyer commented Jan 30, 2019

simon-king-jena commented Jan 30, 2019

sagetrac-git mannequin commented Jan 31, 2019

sagetrac-git mannequin commented Jan 31, 2019

simon-king-jena commented Jan 31, 2019

simon-king-jena commented Jan 31, 2019

jdemeyer commented Jan 31, 2019

jdemeyer commented Jan 31, 2019

vbraun commented Feb 4, 2019