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

Rename top-level makefile to Makefile #10156

Closed
jdemeyer opened this issue Oct 23, 2010 · 35 comments
Closed

Rename top-level makefile to Makefile #10156

jdemeyer opened this issue Oct 23, 2010 · 35 comments

Comments

@jdemeyer
Copy link

This ticket has a few patches which should be applied when renaming makefile to Makefile. See also #9799.

Apply:

  1. attachment: 10156_doc.patch
  2. attachment: 10156_scripts.patch
  3. attachment: install.patch

CC: @jhpalmieri @nexttime

Component: distribution

Keywords: scripts makefile

Author: Jeroen Demeyer, John Palmieri

Reviewer: Volker Braun

Merged: sage-4.6.1.alpha1

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

@jdemeyer jdemeyer added this to the sage-4.6.1 milestone Oct 23, 2010
@jdemeyer
Copy link
Author

Attachment: 10156_doc.patch.gz

sagelib patch

@jdemeyer
Copy link
Author

Attachment: 10156_scripts.patch.gz

sage_scripts patch

@jdemeyer
Copy link
Author

Author: Jeroen Demeyer

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 23, 2010

comment:2

The renaming is also done by #9433 IIRC.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 23, 2010

comment:3

I wonder if it's safer to use [Mm]akefile in the shell scripts. (The scripts calling Python functions rather than os.system() etc. would get more complicated of course.)

At least there seems to be a lack of error / consistency checking, as always...

@nexttime nexttime mannequin added the s: needs review label Oct 23, 2010
@nexttime
Copy link
Mannequin

nexttime mannequin commented Oct 23, 2010

comment:4

Patches look reasonable; I just don't know yet if they catch all instances.

@jdemeyer
Copy link
Author

comment:5

If you thrust the author of the patch to do the test, I am doing the following:

If all this works, I think that #10156 and #10157 are proven to work.

@jdemeyer
Copy link
Author

comment:6

All builds and tests mentioned above worked with #9799, #10156, #10157 applied.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

comment:7

Replying to @nexttime:

I wonder if it's safer to use [Mm]akefile in the shell scripts.

I don't see why. If we really rename makefile to Makefile, there is no reason to use [Mm]akefile.

@jdemeyer
Copy link
Author

jdemeyer commented Nov 1, 2010

Merged: sage-4.6.1.alpha0

@jdemeyer
Copy link
Author

jdemeyer commented Nov 5, 2010

comment:9

John or Leif: could you review this and #10157 please? I think these patches have received sufficient testing to prove that they work.

@jhpalmieri
Copy link
Member

comment:10

The Sage library patch looks fine.

For the scripts patch, I haven't tested it at all, but what happens if you upgrade from a version of Sage with a file called "makefile"? Will things break? I don't think it's ever a good idea to run "sage-sdist" or "sage-bdist" from an upgraded version of Sage, but if someone does, what will happen? Would it be a good idea to either use something like [Mm]akefile, as leif suggested, or do something like the following:

if [ -e makefile ]; then
   cp -$OPT makefile "$TMP"/Makefile
fi

(This is for sage-bdist, and you would use something similar in sage-sdist.)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Nov 5, 2010

comment:11

Replying to @jhpalmieri:

The Sage library patch looks fine.

For the scripts patch, I haven't tested it at all, but what happens if you upgrade from a version of Sage with a file called "makefile"? Will things break?

W.r.t. making new dists from an upgraded version, hopefully #9433 solves this... ;-)

(I don't know if you meant upgrading in general; this works for me. You just still have the old makefile.)

I don't think it's ever a good idea to run "sage-sdist" or "sage-bdist" from an upgraded version of Sage [...]

Nobody should do this, currently.

We should perhaps just give an error message in that case (until #9433 is merged).

@jdemeyer
Copy link
Author

jdemeyer commented Nov 5, 2010

comment:12

I don't understand the fine details of upgrading, but would it be possible to copy makefile to Makefile during the upgrade?

@nexttime
Copy link
Mannequin

nexttime mannequin commented Nov 6, 2010

comment:13

Replying to @jdemeyer:

I don't understand the fine details of upgrading, but would it be possible to copy makefile to Makefile during the upgrade?

Of course. But I would rename it because (POSIX) make gives makefile precedence over Makefile and we don't want to cause confusion.

@jdemeyer
Copy link
Author

jdemeyer commented Nov 7, 2010

comment:14

Replying to @nexttime:

Replying to @jdemeyer:

I don't understand the fine details of upgrading, but would it be possible to copy makefile to Makefile during the upgrade?

Of course. But I would rename it because (POSIX) make gives makefile precedence over Makefile and we don't want to cause confusion.

On the other hand, if makefile and Makefile are identical files, it shouldn't matter. It seems just dangerous to delete makefile during a Sage build/upgrade.

Can any of you implement this?

@jdemeyer
Copy link
Author

jdemeyer commented Nov 7, 2010

Work Issues: copy makefile to Makefile during upgrade

@jhpalmieri
Copy link
Member

comment:15

Replying to @jdemeyer:

Replying to @nexttime:

Replying to @jdemeyer:

I don't understand the fine details of upgrading, but would it be possible to copy makefile to Makefile during the upgrade?

Of course. But I would rename it because (POSIX) make gives makefile precedence over Makefile and we don't want to cause confusion.

On the other hand, if makefile and Makefile are identical files, it shouldn't matter. It seems just dangerous to delete makefile during a Sage build/upgrade.

Note that on OS X, you can't have files called both "makefile" and "Makefile" in the same directory. Also, "makefile" is not used at all during an upgrade, so it should be safe to rename it.

Can any of you implement this?

It's taken care of by #9433. If we want to implement it here, I think it should be done in the sage-upgrade script, after running "./install" and checking its exit status:

diff -r 4047e578febc sage-upgrade
--- a/sage-upgrade      Sat Oct 30 16:05:33 2010 -0700
+++ b/sage-upgrade      Tue Nov 09 10:57:40 2010 -0800
@@ -36,3 +36,13 @@
     exit 1
 fi
 
+cd "$SAGE_ROOT"
+
+if [ -f makefile ]; then
+    mv makefile Makefile
+fi
+
+if [ $? -ne 0 ]; then
+    echo "Error renaming 'makefile' to 'Makefile'.
+    exit 1
+fi

Another option would also check for the existence of 'Makefile' before doing the move:

diff -r 4047e578febc sage-upgrade
--- a/sage-upgrade      Sat Oct 30 16:05:33 2010 -0700
+++ b/sage-upgrade      Tue Nov 09 10:58:47 2010 -0800
@@ -36,3 +36,16 @@
     exit 1
 fi
 
+cd "$SAGE_ROOT"
+
+if [ -f makefile ]; then
+    if [ -f Makefile ]; then
+       mv Makefile Makefile.old
+    fi
+    mv makefile Makefile
+fi
+
+if [ $? -ne 0 ]; then
+    echo "Error renaming 'makefile' to 'Makefile'.
+    exit 1
+fi

Opinions either way?

@jdemeyer
Copy link
Author

Attachment: 10156_upgrade.patch.gz

Additional scripts patch: rename makefile to Makefile during upgrade

@jdemeyer
Copy link
Author

Changed merged from sage-4.6.1.alpha0 to none

@jdemeyer
Copy link
Author

Merged: sage-4.6.1.alpha1

@jhpalmieri
Copy link
Member

comment:18

The first two patches (10156_docs.patch and 10156_scripts.patch) get a positive review from me.

This doesn't work, probably my fault. The issue is that the script "sage-upgrade" doesn't get upgraded in the process. So I think moving makefile should happen at the end of the "install" script instead, since this gets downloaded before it gets run. (In particular, upgrading didn't rename "makefile" when I tested it.) Try this patch to "install" instead.

@jhpalmieri
Copy link
Member

Changed author from Jeroen Demeyer to Jeroen Demeyer, John Palmieri

@jhpalmieri
Copy link
Member

Attachment: install.gz

the file spkg/install

@jhpalmieri
Copy link
Member

Attachment: install.patch.gz

patch for 'install'

@jhpalmieri
Copy link
Member

comment:19

Replying to @jhpalmieri:

This doesn't work, probably my fault.

By "this", I meant the third patch, 10156_upgrade.patch.

@jhpalmieri
Copy link
Member

comment:20

For what it's worth, my change to "install" works for me when upgrading on both Mac OS X (with its odd case-sensitivity) and Solaris. I tested upgrading from 4.5.3 (which had "makefile") and 4.6.1.alpha0 (which had "Makefile").

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Changed work issues from copy makefile to Makefile during upgrade to none

@jdemeyer jdemeyer changed the title Rename makefile to Makefile Rename top-level makefile to Makefile Nov 15, 2010
@vbraun
Copy link
Member

vbraun commented Nov 16, 2010

Reviewer: Volker Braun

@vbraun
Copy link
Member

vbraun commented Nov 16, 2010

comment:23

I successfully updated from sage-4.5.2 on Fedora 13 and the patch did go the extra mile to make sure that it doesn't break upgrades. I'll set it to positive review.

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Nov 16, 2010

comment:25

It looks to me there are still a few files that refer to makefile. Shall I create a ticket for these:

devel/sage-main/doc/output/latex/en/developer/developer.tex:Another way is to edit the file \code{makefile} in the top level Sage
devel/sage-main/doc/output/latex/en/developer/developer.tex:After saving all changes to \code{makefile}, we can parallel test with the
devel/sage-main/doc/output/latex/en/developer/developer.tex:these command, see the files \code{SAGE\_ROOT/makefile} and
devel/sage-main/doc/output/latex/en/developer/developer.tex:argument \code{-long}. See the file \code{SAGE\_ROOT/makefile} for further
devel/sage-main/doc/output/latex/en/developer/developer.tex:\code{SAGE\_ROOT/makefile} for further details.
devel/sage-main/doc/output/latex/en/developer/developer.tex:the file \code{SAGE\_ROOT/makefile} takes care of the unpacking,
devel/sage-main/doc/output/latex/en/developer/developer.tex:\code{\$SAGE\_ROOT/makefile} before using \code{ptestlong}). See

@jdemeyer
Copy link
Author

comment:26

Replying to @sagetrac-drkirkby:

It looks to me there are still a few files that refer to makefile. Shall I create a ticket for these:

devel/sage-main/doc/output/latex/en/developer/developer.tex:Another way is to edit the file \code{makefile} in the top level Sage
devel/sage-main/doc/output/latex/en/developer/developer.tex:After saving all changes to \code{makefile}, we can parallel test with the
devel/sage-main/doc/output/latex/en/developer/developer.tex:these command, see the files \code{SAGE\_ROOT/makefile} and
devel/sage-main/doc/output/latex/en/developer/developer.tex:argument \code{-long}. See the file \code{SAGE\_ROOT/makefile} for further
devel/sage-main/doc/output/latex/en/developer/developer.tex:\code{SAGE\_ROOT/makefile} for further details.
devel/sage-main/doc/output/latex/en/developer/developer.tex:the file \code{SAGE\_ROOT/makefile} takes care of the unpacking,
devel/sage-main/doc/output/latex/en/developer/developer.tex:\code{\$SAGE\_ROOT/makefile} before using \code{ptestlong}). See

These are all in the output directory. Could it be that you did not rebuild the pdf documentation after applying these tickets and are finding the old tex files?

@sagetrac-drkirkby
Copy link
Mannequin

sagetrac-drkirkby mannequin commented Nov 16, 2010

comment:27

Replying to @jdemeyer:

These are all in the output directory. Could it be that you did not rebuild the pdf documentation after applying these tickets and are finding the old tex files?

Yes, it is very possible. I'm just going to build Sage again from scratch, as I've messed around with various files. But I very much doubt I built the pdfs, so that explains it.

Dave

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

3 participants