Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#21103 closed defect (fixed)

Update rubiks' patches to conform to same format as other patches

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.4
Component: packages: standard Keywords:
Cc: embray Merged in:
Authors: Erik Bray, Jeroen Demeyer Reviewers: Jeroen Demeyer, Erik Bray, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 87963a9 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The rubiks package is copying files instead of proper patching. Clean this up.

See also #20837 and #20933 which did the same for other packages.

Change History (18)

comment:1 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/update_rubiks__patches_to_conform_to_same_format_as_other_patches

comment:2 Changed 4 years ago by git

  • Commit set to a1a09b844984a8dbb9bd5597f27507ccd206d787

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

a1a09b8Force rebuild of rubiks

comment:3 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 4 years ago by jdemeyer

I am not really convinced that the non-patched Makefiles are fine. Surely, the person who patched them must have had a reason for it. There is for example this comment in build/pkgs/rubiks/patches/dietz-solver-Makefile:

# This Makefile was seriously broken.
# CC was set to g++. Since it was compiling C++ files,
# CXX should have been used.
# LINK was set to g++, so I changed that to LD
# CFLAGS was set to -O2. I've removed that, so it can be set
# in spkg-install.
# In any case, it should have been CXXFLAGS
# LFLAGS and INCLUDES were both empty
# David Kirkby, 29th Sept 2009

comment:6 Changed 4 years ago by embray

Compare to the current source. For the makefiles I removed from the patch there is no longer any difference from upstream (which has probably been patched).

comment:7 Changed 4 years ago by embray

Actually scratch that, I was confused. I don't think those patches were supposed to be removed. You can update the commit to not remove the makefile patches (just the full makefile copies).

Last edited 4 years ago by embray (previous) (diff)

comment:8 Changed 4 years ago by mkoeppe

With the branch on this ticket, sage -f rubiks and sage -t src/sage/interfaces/rubik.py go through (on Mac OS X).

What is considered upstream for this package? Should there be an spkg-src? The links in http://doc.sagemath.org/html/en/reference/interfaces/sage/interfaces/rubik.html and in SPKG.txt are broken.

comment:9 Changed 4 years ago by embray

I think all this needs to is roll back the removal of

  • build/pkgs/rubiks/patches/dietz-cu2-Makefile.patch
  • build/pkgs/rubiks/patches/dietz-mcube-Makefile.patch
  • build/pkgs/rubiks/patches/dietz-solver-Makefile.patch

from the commit. They shouldn't have been removed.

comment:10 Changed 4 years ago by mkoeppe

  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from needs_review to needs_work

comment:11 Changed 4 years ago by git

  • Commit changed from a1a09b844984a8dbb9bd5597f27507ccd206d787 to 87963a9abc2ae28fd8ad15bade4e1f22eddad087

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

fb539e1Clean up patching of rubiks
98cec94Fix patch loop
87963a9Force rebuild of rubiks

comment:12 Changed 4 years ago by jdemeyer

  • Authors changed from Erik Bray to Erik Bray, Jeroen Demeyer
  • Reviewers set to Jeroen Demeyer, Erik Bray
  • Status changed from needs_work to needs_review

This now works for me. If you're happy with this, then you can it to positive_review.

comment:13 follow-up: Changed 4 years ago by mkoeppe

comment 8.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mkoeppe:

comment 8.

I don't really care about that. It's unrelated to this ticket anyway.

comment:15 in reply to: ↑ 14 Changed 4 years ago by mkoeppe

Replying to jdemeyer:

I don't really care about that. It's unrelated to this ticket anyway.

Fine, I've created a new ticket #21493 for that.

comment:16 Changed 4 years ago by mkoeppe

  • Reviewers changed from Jeroen Demeyer, Erik Bray to Jeroen Demeyer, Erik Bray, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:17 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/update_rubiks__patches_to_conform_to_same_format_as_other_patches to 87963a9abc2ae28fd8ad15bade4e1f22eddad087
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 4 years ago by embray

  • Commit 87963a9abc2ae28fd8ad15bade4e1f22eddad087 deleted

Thanks!

Note: See TracTickets for help on using tickets.