sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.39k stars 473 forks source link

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

Closed jdemeyer closed 8 years ago

jdemeyer commented 8 years ago

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.

CC: @embray

Component: packages: standard

Author: Erik Bray, Jeroen Demeyer

Branch: 87963a9

Reviewer: Jeroen Demeyer, Erik Bray, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/21103

jdemeyer commented 8 years ago

Branch: u/jdemeyer/update_rubiks__patches_to_conform_to_same_format_as_other_patches

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

a1a09b8Force rebuild of rubiks
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Commit: a1a09b8

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
+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.
jdemeyer commented 8 years ago
comment:5

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
embray commented 8 years ago
comment:6

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).

embray commented 8 years ago
comment:7

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).

mkoeppe commented 8 years ago
comment:8

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.

embray commented 8 years ago
comment:9

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

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from a1a09b8 to 87963a9

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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
jdemeyer commented 8 years ago
comment:12

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

jdemeyer commented 8 years ago

Changed author from Erik Bray to Erik Bray, Jeroen Demeyer

jdemeyer commented 8 years ago

Reviewer: Jeroen Demeyer, Erik Bray

mkoeppe commented 8 years ago
comment:13

comment 8.

jdemeyer commented 8 years ago
comment:14

Replying to @mkoeppe:

comment 8.

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

mkoeppe commented 8 years ago
comment:15

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.

mkoeppe commented 8 years ago

Changed reviewer from Jeroen Demeyer, Erik Bray to Jeroen Demeyer, Erik Bray, Matthias Koeppe

vbraun commented 8 years ago

Changed branch from u/jdemeyer/update_rubiks__patches_to_conform_to_same_format_as_other_patches to 87963a9

embray commented 8 years ago
comment:18

Thanks!

embray commented 8 years ago

Changed commit from 87963a9 to none