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 )
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:
- use
$SAGE_LOCAL/etc/sage-64.txt
as indicator file forSAGE64
instead of$SAGE_LOCAL/lib/sage-64.txt
(sage-64.txt
looks more like a configuration file than a library). - remove all "echo" statements from
sage-check-64
(they were being redirected to /dev/null anyway). - 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. - remove sourcing of
sage-check-64
fromlocal/bin/sage-build
. The latter script is only called fromspkg/bin/sage
at which pointSAGE64
is already set if needed. - 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). - if SAGE64 is set to "no", delete sage-64.txt.
Apply:
- 12789_scripts.patch to the SCRIPTS repository.
- 12789_root.patch to the ROOT repository.
Attachments (2)
Change History (19)
comment:1 Changed 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
- Description modified (diff)
comment:5 Changed 7 years ago by
- Status changed from new to needs_review
comment:6 Changed 7 years ago by
- Description modified (diff)
comment:7 follow-up: ↓ 8 Changed 7 years ago by
comment:8 in reply to: ↑ 7 Changed 7 years ago by
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
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:10 follow-up: ↓ 11 Changed 7 years ago by
- 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: ↓ 14 Changed 7 years ago by
Replying to jdemeyer:
Added items 5 and 6.
One idea from #10303 that I disagree with is to source
sage-check-64
(nowsage-arch-env
) only insage-spkg
andsage-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: ↓ 13 Changed 7 years ago by
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
Replying to leif:
Shouldn't there be at least a warning if
SAGE64
is set to"no"
butsage-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
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
- Description modified (diff)
Changed 7 years ago by
Changed 7 years ago by
comment:16 Changed 7 years ago by
- 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
- Merged in set to sage-5.1.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
I haven't looked at the patches here, but #10303 and #11077 also had some ideas for clean up for
sage-check-64
.