Opened 4 years ago

Closed 3 years ago

#23733 closed enhancement (fixed)

Stop supporting SAGE64 and CFLAG64

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-8.2
Component: build Keywords:
Cc: dimpase Merged in:
Authors: John Palmieri, Jeroen Demeyer Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 905e4d4 (Commits, GitHub, GitLab) Commit: 905e4d44a3da6d0265c9d2ae24d19691f40c0458
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

I propose to get rid of src/bin/sage-arch-env and $SAGE_LOCAL/etc/sage-64.txt, which is a very specialized mechanism of storing a bit of configuration in SAGE_LOCAL. Instead, SAGE64 should simply be replaced by adding -m64 in $CC or $CFLAGS and maybe setting ABI=64. See #24596.

Since CFLAG64 is only used to support SAGE64, this implies that CFLAG64 is no longer used either.

Change History (46)

comment:1 in reply to: ↑ description ; follow-up: Changed 4 years ago by jdemeyer

Replying to mkoeppe:

Instead, SAGE64 should become a configure option (see #23570).

Is anybody actually using that option? I would go for deprecating it and getting rid of it eventually.

comment:2 in reply to: ↑ 1 Changed 4 years ago by mkoeppe

Replying to jdemeyer:

Is anybody actually using that option? I would go for deprecating it and getting rid of it eventually.

Good idea. Should as well get rid of it in the installation manual eventually (it mentions some ancient Mac OS versions, for which probably no working build infrastructure exists any more anyway).

comment:3 Changed 4 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/no-sage64

comment:4 Changed 4 years ago by jhpalmieri

  • Commit set to 9b58204f1c96dabb69b8b7424ce9f69b283cadfb

I added two deprecation warnings: once at the beginning and once at the end of the file build/make/install: at the beginning so that it appears near the start and is there if someone interrupts the build with ctrl-C, and at the end so that it is clearly displayed. (If I put it in sage-arch-env instead, it gets buried with all of the other build messages.) I tested with SAGE64=yes make.

I asked on sage-devel whether anyone is using this. Let's wait a bit for responses.


New commits:

9b58204trac 23733: deprecate SAGE64 and CFLAG64

comment:5 Changed 4 years ago by jhpalmieri

From the few responses on sage-devel so far, I think that SAGE64 is only useful on Solaris. Do we keep the variable? (Do we keep "supporting" Solaris?) Can it be merged into some configure option, in which the system type is detected and -m64 is added to the appropriate flags automatically?

comment:6 Changed 4 years ago by drkirkby

I don't believe that SAGE64 only adds -m64, as if that had been the case, there are obviously easier ways to do it. Adding -m64 is the case in some packages, but not all of them.

Dave

comment:7 Changed 4 years ago by dimpase

We now have access to a SPARC T4 running Solaris 11, things don't look too bad for Solaris at the moment. I've opened #24596 to track this.

Last edited 4 years ago by dimpase (previous) (diff)

comment:8 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-8.1 to sage-8.2
  • Summary changed from Get rid of src/bin/sage-arch-env, SAGE_LOCAL/etc/sage-64.txt to Stop supporting SAGE64

comment:9 Changed 4 years ago by jdemeyer

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

comment:10 Changed 4 years ago by jdemeyer

  • Branch changed from u/jhpalmieri/no-sage64 to u/jdemeyer/no-sage64

comment:11 Changed 4 years ago by jdemeyer

  • Commit changed from 9b58204f1c96dabb69b8b7424ce9f69b283cadfb to 0ad41abda20e2c5fac12e2f78a12e66ac10f6d1b
  • Description modified (diff)

New commits:

6eeec43trac 23733: deprecate SAGE64 and CFLAG64
0ad41abStop supporting SAGE64

comment:12 Changed 4 years ago by git

  • Commit changed from 0ad41abda20e2c5fac12e2f78a12e66ac10f6d1b to fbd7feeb6a6346d5d57cd8a741ef6730ff791686

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fbd7feeStop supporting SAGE64

comment:13 Changed 4 years ago by jdemeyer

  • Authors changed from Jeroen Demeyer to John Palmieri, Jeroen Demeyer
  • Cc dimpase added; jdemeyer removed
  • Status changed from new to needs_review

comment:14 follow-up: Changed 4 years ago by dimpase

Do we need ABI=64 set on Solaris 64-bit, or not?

comment:15 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

Replying to dimpase:

Do we need ABI=64 set on Solaris 64-bit, or not?

No idea. So far I have not been setting it and it seems to work like that.

comment:16 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:17 Changed 4 years ago by mkoeppe

(Disclaimer: Last time I used Solaris was 10 years ago.) The only time that I remember using the ABI environment variable was for configuring GMP if one wants to do a 32-bit build on 64-bit hardware - a quirk of GMP's configuration scripts. I don't think it's a standard variable at all.

comment:18 follow-ups: Changed 4 years ago by dimpase

A proper way to do this is to allow something like --abi=32/64 (or ABI=32/64) to ./configure, and not doing dodgy and error-prone setting of CFLAGS, CXXFLAGS, FCFLAGS, FFLAGS, LDFLAGS, etc etc to take -m64/32.

In particular, ABI and -m64 should persist, i.e. written by ./configure to the Makefile or something like this.

Last edited 4 years ago by dimpase (previous) (diff)

comment:19 follow-up: Changed 4 years ago by dimpase

As well, numpy's (and all the other packages that like to test stuff without CFLAGS etc set to non-empty values) spkg-install will break, as -m64 won't be passed. The current version of this spkg-install modifies CC to include the flag.

We cannot and should not modify CC globally to have flags in them, as there might be places where CC is assumed not to have spaces (GAP is one such place, I think).

comment:20 Changed 4 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to needs_work

comment:21 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to dimpase:

dodgy and error-prone setting of CFLAGS, CXXFLAGS, FCFLAGS, FFLAGS, LDFLAGS, etc etc to take -m64/32.

These are very standard flags. Setting them is a normal thing.

comment:22 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Replying to dimpase:

A proper way to do this is to allow something like --abi=32/64 (or ABI=32/64) to ./configure

In particular, ABI and -m64 should persist, i.e. written by ./configure to the Makefile or something like this.

Of course. But adding ./configure options is outside the scope of this ticket. This ticket is about removing something which is unmaintained and almost certainly broken.

comment:23 in reply to: ↑ 19 Changed 4 years ago by jdemeyer

Replying to dimpase:

As well, numpy's (and all the other packages that like to test stuff without CFLAGS etc set to non-empty values) spkg-install will break, as -m64 won't be passed. The current version of this spkg-install modifies CC to include the flag.

We cannot and should not modify CC globally to have flags in them, as there might be places where CC is assumed not to have spaces (GAP is one such place, I think).

I would argue that it's simply better to build/use a GCC version which has the correct flags by default.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:24 follow-up: Changed 4 years ago by dimpase

this patch makes it impossible to build Sage in one go. I have already explained why, cause you cannot set CC to gcc -m64 as this will break GAP, but otherwise you will break numpy...

comment:25 in reply to: ↑ 24 Changed 4 years ago by jdemeyer

Replying to dimpase:

this patch makes it impossible to build Sage in one go.

First of all, I don't know what you mean with "build Sage in one go".

Second, regardless of what you mean: is it a regression? Suppose that the following are true:

(A) It is impossible to build Sage in one go without this patch.

(B) It is impossible to build Sage in one go with this patch.

In these are both true (since I don't know what you mean, I can't say whether that is the case), then your complaint is not meaningful.

comment:26 follow-up: Changed 4 years ago by dimpase

I am saying that with this patch on a platform that needs SAGE64 set one will not be able to get Sage built by running configure+make.

The build will break, at some point you will need to build individual packages with adjusted flags.

comment:27 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to dimpase:

I am saying that with this patch on a platform that needs SAGE64 set one will not be able to get Sage built by running configure+make.

OK. I am saying that without this patch on a platform that needs SAGE64 set one will not be able to get Sage built by running configure+make. So, this patch doesn't break anything that wasn't already broken before.

comment:28 Changed 4 years ago by dimpase

I am saying that it breaks more than it fixes, e.g. it removes all the traces of the fact that numpy needs a special treatment.

comment:29 Changed 4 years ago by git

  • Commit changed from fbd7feeb6a6346d5d57cd8a741ef6730ff791686 to 9f5a57fe906f1ddd7bcf6ed0bf57263824f3265e

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

9f5a57fKeep SAGE64 for numpy

comment:30 Changed 4 years ago by jdemeyer

OK fine. I've kept the special-casing for numpy.

comment:31 Changed 4 years ago by dimpase

In Atlas configuration you did the following:

-conf['32bit?'] = not conf['64bit?']
+conf['64bit?'] = (conf['bits'] == '64bit')
+conf['32bit?'] = (conf['bits'] == '32bit')

This is a different logic for conf['32bit?'] vs. the originally intended one. By right, you should have just done

+conf['64bit?'] = (conf['bits'] == '64bit')
conf['32bit?'] = not conf['64bit?']
...

comment:32 Changed 4 years ago by git

  • Commit changed from 9f5a57fe906f1ddd7bcf6ed0bf57263824f3265e to a66a32e9acf36ab6038491a29aeb4690e6c793af

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

a66a32eRevert 32bit configuration

comment:33 Changed 4 years ago by jdemeyer

This reverts the ATLAS logic.

comment:34 follow-up: Changed 4 years ago by dimpase

What does this patch do to CFLAG64? (which is a documented Sage variable after all).

It's not clear. The ticket description does not mention it, but in several places you remove special-casing for it.

comment:35 Changed 4 years ago by jdemeyer

  • Summary changed from Stop supporting SAGE64 to Stop supporting SAGE64 and CFLAG64

comment:36 in reply to: ↑ 34 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:37 Changed 4 years ago by git

  • Commit changed from a66a32e9acf36ab6038491a29aeb4690e6c793af to ded617f86815657dded89ab7e5801d383cf67ecf

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9a662f7trac 23733: deprecate SAGE64 and CFLAG64
ded617fStop supporting SAGE64 except in Numpy

comment:38 in reply to: ↑ 18 Changed 4 years ago by jdemeyer

Replying to dimpase:

A proper way to do this is to allow something like --abi=32/64 (or ABI=32/64) to ./configure

I created #24672 for that.

comment:39 Changed 3 years ago by embray

Since the work I'm doing in #24024 will likely conflict heavily with this, I'm trying to remove SAGE64 support from individual packages as I go. So that work can basically merged with this. I'll try to pay attention to what changes this is making to the relevant packages.

Alternatively we merge this now, and I rebase everything on top of this.

comment:40 Changed 3 years ago by embray

Also, if you would prefer to just do the latter, then positive review from me.

comment:41 follow-up: Changed 3 years ago by dimpase

I suppose this needs a bit more testing. Should we try to put together a new branch for Solaris and try this out there?

comment:42 Changed 3 years ago by git

  • Commit changed from ded617f86815657dded89ab7e5801d383cf67ecf to 905e4d44a3da6d0265c9d2ae24d19691f40c0458

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b4ecda9trac 23733: deprecate SAGE64 and CFLAG64
905e4d4Stop supporting SAGE64 except in Numpy

comment:43 in reply to: ↑ 41 Changed 3 years ago by jdemeyer

Replying to dimpase:

I suppose this needs a bit more testing. Should we try to put together a new branch for Solaris and try this out there?

Fine, I just rebased it to 8.2.rc0.

comment:44 Changed 3 years ago by dimpase

  • Status changed from needs_review to positive_review

OK, this works on 64-bit SPARC Solaris.

comment:45 Changed 3 years ago by embray

Just as a note, this will currently conflict with most of the tickets listed under

#14804
transition to using install program
#25035
Use sage-dist-helpers + add DESTDIR support for flint and arb
#25037
Add destdir support and other cleanup for ntl
#25038
Use sage-dist-helpers for curl and gc
#25039
Add sdh_install helper function to sage-dist-helpers
#25040
Update additional packages to use sdh_install
#25042
Add DESTDIR support for freetype
#25043
Add DESTDIR support for python2/python3
#25044
Add DESTDIR support for gap
#25045
Add DESTDIR support for mpfr
#25048
Add DESTDIR support for tachyon, and other cleanup
#25051
Add DESTDIR support to additional Python packages; upgrade pip to latest patch release
#25052
Add DESTDIR support for openblas
#25085
Add DESTDIR support for zn_poly, and additional cleanup
#25086
Add DESTDIR support for nauty
#25087
Add DESTDIR support for ppl
#25099
Add DESTDIR support for sympow
#25100
Add DESTDIR support for gfan, lcalc, ratpoints, rubiks, symmetrica
#25142
Add DESTDIR support for zlib
#25143
Add DESTDIR support for pari
#25144
Add DESTDIR support for palp
#25147
Add DESTDIR support for boost_cropped
#25372
Add DESTDIR support for cryptominisat
#25851
Add -C option and DESTDIR to make
#27016
DESTDIR support for gcc, improvements to gfortran
#27039
Add DESTDIR support for optional packages that use autotools
#28927
DESTDIR support for libbraiding and libhomfly
#29148
Add DESTDIR and Cygwin support for mcqd
#29150
DESTDIR support for coxeter3

However, I will merge this branch into those tickets and add this ticket as a dependency. This one should definitely be merged first though.

comment:46 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/no-sage64 to 905e4d44a3da6d0265c9d2ae24d19691f40c0458
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.