Opened 9 years ago

Closed 9 years ago

#12799 closed enhancement (fixed)

Fix PolyBoRi's dependencies in `module_list.py`

Reported by: AlexanderDreyer Owned by: AlexanderDreyer
Priority: major Milestone: sage-5.0
Component: build Keywords:
Cc: leif Merged in: sage-5.0.beta13
Authors: Alexander Dreyer Reviewers: Leif Leonhardy
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by AlexanderDreyer)

Actually only the pbori module needs to get rebuilt; while touching $SAGE_LOCAL/include/polybori.h doesn't help, touching $SAGE_LOCAL/include/polybori/* does (afterwards running ./sage -b).

The better alternative is to fix the dependency list in module_list.py

Current patch

Attachments (3)

polybori_depends.patch (1.1 KB) - added by AlexanderDreyer 9 years ago.
Patch for the sage library (devel/sage)
polybori_depends.2.patch (930 bytes) - added by AlexanderDreyer 9 years ago.
Patch for the sage library (devel/sage)
polybori_depends.3.patch (930 bytes) - added by AlexanderDreyer 9 years ago.
patch rebased to sage-5.0 beta11

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by AlexanderDreyer

Patch for the sage library (devel/sage)

comment:1 Changed 9 years ago by AlexanderDreyer

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by leif

  • Authors changed from AlexanderDreyer to Alexander Dreyer
  • Description modified (diff)
  • Reviewers set to Leif Leonhardy

As mentioned on #12655, I'd rather make the extension module depend on just $SAGE_INC/polybori.h and/or its libraries, $SAGE_LOCAL/lib/{polybori,polybori_groebner}.so, and touch the former from spkg-install at least in case you don't add the libraries.

AFAIK Cython is smart enough to notice when (at least directly) included headers are newer, but this doesn't help if their timestamps are preserved during PolyBoRi's installation.

Adding all of PolyBoRi's headers explicitly seems a bit overkill to me.

[Also note that we have both $SAGE_INC/polybori.h and $SAGE_INC/polybori/polybori.h, which are not the same files.]

comment:3 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by AlexanderDreyer

Replying to leif:

As mentioned on #12655, I'd rather make the extension module depend on just $SAGE_INC/polybori.h and/or its libraries, $SAGE_LOCAL/lib/{polybori,polybori_groebner}.so, and touch the former from spkg-install at least in case you don't add the libraries.

The timestamp of include/polybori/config.h also varies, this would also help here.

AFAIK Cython is smart enough to notice when (at least directly) included headers are newer, but this doesn't help if their timestamps are preserved during PolyBoRi's installation.

Adding all of PolyBoRi's headers explicitly seems a bit overkill to me.

[Also note that we have both $SAGE_INC/polybori.h and $SAGE_INC/polybori/polybori.h, which are not the same files.]

The first header is just for convenience.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 9 years ago by leif

Replying to AlexanderDreyer:

The timestamp of include/polybori/config.h also varies, this would also help here.

Then I'd add just that... (cross-posting)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by AlexanderDreyer

Replying to leif:

Replying to AlexanderDreyer:

The timestamp of include/polybori/config.h also varies, this would also help here.

Then I'd add just that... (cross-posting)

Ok, let's try this one: polybori_depends.2.patch

Changed 9 years ago by AlexanderDreyer

Patch for the sage library (devel/sage)

comment:6 Changed 9 years ago by AlexanderDreyer

  • Description modified (diff)

comment:7 in reply to: ↑ 5 Changed 9 years ago by leif

Replying to AlexanderDreyer:

Ok, let's try this one: polybori_depends.2.patch

Looks better.

comment:8 follow-ups: Changed 9 years ago by leif

  • Component changed from algebra to build
  • Status changed from needs_review to positive_review

Applies to Sage 5.0.beta11 with three lines offset, but I don't mind.

Changed 9 years ago by AlexanderDreyer

patch rebased to sage-5.0 beta11

comment:9 in reply to: ↑ 8 Changed 9 years ago by AlexanderDreyer

  • Description modified (diff)

Replying to leif:

Applies to Sage 5.0.beta11 with three lines offset, but I don't mind.

Thanks! I rebased the patch accordingly.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by jdemeyer

Replying to leif:

Applies to Sage 5.0.beta11 with three lines offset, but I don't mind.

I consider an offset in a patch to be totally harmless; fuzz 1 should be rebased but I can usually live with; fuzz 2 must be rebased.

comment:11 in reply to: ↑ 10 Changed 9 years ago by leif

Replying to jdemeyer:

Replying to leif:

Applies to Sage 5.0.beta11 with three lines offset, but I don't mind.

I consider an offset in a patch to be totally harmless; fuzz 1 should be rebased but I can usually live with; fuzz 2 must be rebased.

Yes, I should perhaps have said that... ;-)

comment:12 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta13
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.