Opened 7 years ago

Closed 7 years ago

#12789 closed enhancement (fixed)

Move local/bin/sage-check-64 to spkg/bin/sage-arch-env

Reported by: jdemeyer Owned by: leif
Priority: major Milestone: sage-5.1
Component: scripts Keywords: sd40.5
Cc: Merged in: sage-5.1.beta4
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Since sage-env sources $SAGE_LOCAL/bin/sage-check-64, it makes sense to move the latter file to $SAGE_ROOT/spkg/bin. I also propose to rename the file to "sage-arch-env". I would like to support a SAGE32 flag (see #12726) and it makes sense to use the same file for this.

Further changes:

  1. use $SAGE_LOCAL/etc/sage-64.txt as indicator file for SAGE64 instead of $SAGE_LOCAL/lib/sage-64.txt (sage-64.txt looks more like a configuration file than a library).
  2. remove all "echo" statements from sage-check-64 (they were being redirected to /dev/null anyway).
  3. move the sourcing of this file up in sage-env, before the compiler flags are set. This would allow changing the default compiler flags, depending on SAGE64 or other future architecture flags.
  4. remove sourcing of sage-check-64 from local/bin/sage-build. The latter script is only called from spkg/bin/sage at which point SAGE64 is already set if needed.
  5. remove all uname checks: don't make a difference between OS X, Solaris and other systems (it doesn't mean SAGE64 is supported on all systems of course).
  6. if SAGE64 is set to "no", delete sage-64.txt.

Apply:

  1. 12789_scripts.patch to the SCRIPTS repository.
  2. 12789_root.patch to the ROOT repository.

Attachments (2)

12789_scripts.patch (2.2 KB) - added by jdemeyer 7 years ago.
12789_root.patch (2.4 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:2 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:6 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:7 follow-up: Changed 7 years ago by jhpalmieri

I haven't looked at the patches here, but #10303 and #11077 also had some ideas for clean up for sage-check-64.

Last edited 7 years ago by jhpalmieri (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 7 years ago by jdemeyer

Replying to jhpalmieri:

I haven't looked at the patches here, but #10303 and #11077 also had some ideas for clean up for sage-check-64.

Too bad those tickets got abandoned, as they are bitrotten now. Anyway, I will look at those tickets and take the good ideas here.

comment:9 Changed 7 years ago by jdemeyer

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

comment:10 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Added items 5 and 6.

One idea from #10303 that I disagree with is to source sage-check-64 (now sage-arch-env) only in sage-spkg and sage-build, i.e. when a package is being built. I think it can be useful even within Sage, for example to compile Cython files.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by leif

Replying to jdemeyer:

Added items 5 and 6.

One idea from #10303 that I disagree with is to source sage-check-64 (now sage-arch-env) only in sage-spkg and sage-build, i.e. when a package is being built. I think it can be useful even within Sage, for example to compile Cython files.

Well, Cython should take CFLAGS etc. from distutils, i.e., Python.

Doesn't really make sense to change the settings (SAGE64 or whatever) afterwards or inbetween. Either you do (or have) a 64-bit build, or not. The original purpose of the sage-64.txt file is exactly to avoid mixing up 32-bit and 64-bit builds (although there are certainly better ways to achieve this).

comment:12 follow-up: Changed 7 years ago by leif

Shouldn't there be at least a warning if SAGE64 is set to "no" but sage-64.txt already exists?

Since this is exactly what we want to avoid: Mixing up both.

comment:13 in reply to: ↑ 12 Changed 7 years ago by jdemeyer

Replying to leif:

Shouldn't there be at least a warning if SAGE64 is set to "no" but sage-64.txt already exists?

I believe in the philosophy of "user is always right". If the user intentionally set SAGE64=yes and then later SAGE64=no, we assume he knows what he's doing. I don't see the need for a warning. Besides, the warning would only be printed once (sage-64.txt is deleted if SAGE64=no), so it would be easy to miss.

comment:14 in reply to: ↑ 11 Changed 7 years ago by jdemeyer

Replying to leif:

Well, Cython should take CFLAGS etc. from distutils, i.e., Python.

Perhaps, but I think it's not impossible that some Sage library code or user code wants to look at SAGE64. However, I'm not going to insist on this. I just want this ticket to get reviewed and merged, unlike #10303.

comment:15 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Changed 7 years ago by jdemeyer

Changed 7 years ago by jdemeyer

comment:16 Changed 7 years ago by vbraun

  • Keywords sd40.5 added
  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good!

comment:17 Changed 7 years ago by jdemeyer

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