Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jdemeyer opened this issue Jul 27, 2016 · 21 comments
Closed

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

jdemeyer opened this issue Jul 27, 2016 · 21 comments

Comments

@jdemeyer
Copy link

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
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2016

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

a1a09b8Force rebuild of rubiks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 27, 2016

Commit: a1a09b8

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

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
Copy link
Contributor

embray commented Jul 27, 2016

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
Copy link
Contributor

embray commented Jul 27, 2016

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
Copy link
Member

mkoeppe commented Aug 3, 2016

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
Copy link
Contributor

embray commented Aug 16, 2016

comment:9

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2016

Changed commit from a1a09b8 to 87963a9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2016

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
Copy link
Author

comment:12

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

@jdemeyer
Copy link
Author

Changed author from Erik Bray to Erik Bray, Jeroen Demeyer

@jdemeyer
Copy link
Author

Reviewer: Jeroen Demeyer, Erik Bray

@mkoeppe
Copy link
Member

mkoeppe commented Sep 14, 2016

comment:13

comment 8.

@jdemeyer
Copy link
Author

comment:14

Replying to @mkoeppe:

comment 8.

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

@mkoeppe
Copy link
Member

mkoeppe commented Sep 14, 2016

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
Copy link
Member

mkoeppe commented Sep 14, 2016

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

@vbraun
Copy link
Member

vbraun commented Sep 17, 2016

@embray
Copy link
Contributor

embray commented Sep 22, 2016

comment:18

Thanks!

@embray
Copy link
Contributor

embray commented Sep 22, 2016

Changed commit from 87963a9 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants