Opened 9 years ago

Closed 5 years ago

Last modified 5 years ago

#13044 closed defect (fixed)

ecl chokes when CC or CXX contains space

Reported by: ohanar Owned by: GeorgSWeber
Priority: major Milestone: sage-7.2
Component: packages: standard Keywords: sd40.5
Cc: Merged in:
Authors: R. Andrew Ohana, Erik Bray, Jeroen Demeyer Reviewers: Jeroen Demeyer, Erik Bray
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 9049b15 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Currently ecl completely fails to build when CC or CXX contains a space. This adds a patch accepted by upstream to fix this.

Upstream (patched): http://sage.ugent.be/www/jdemeyer/sage/ecl-15.3.7.tar.bz2

Change History (33)

comment:1 Changed 9 years ago by ohanar

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

comment:2 Changed 9 years ago by ohanar

  • Description modified (diff)
  • Summary changed from ccache chokes on ecl to ecl chokes when CC or CXX contains space

comment:3 Changed 9 years ago by ddrake

I can verify that our current ECL chokes with "env CC='nice gcc' ./spkg -i ecl..." and that your spkg fixes it. Perhaps it would be nice to put something into the generated ecl_cc so we know whence it came: maybe a comment like # ECL chokes when CC contains spaces; this script works around that. See #13044. Then it would be easier to remove this workaround when ECL is updated.

Also, I tried env CXX='nice g++' ./sage -f ecl... with the old version and it worked fine. Perhaps we shouldn't also create ecl_cxx since ECL is not using a C++ compiler.

Also^2, we should send this upstream.

Last edited 9 years ago by ddrake (previous) (diff)

comment:4 Changed 8 years ago by jdemeyer

  • Component changed from build to packages

comment:5 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 Changed 7 years ago by fbissey

  • Status changed from needs_review to needs_info

Is this still needed?

comment:7 Changed 7 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Yes, it is still needed.

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 7 years ago by pbruin

  • Status changed from needs_review to needs_work

This needs to be converted to a new-style package, for the ECL version we currently use.

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:12 Changed 5 years ago by embray

I can confirm this is still an issue--sage build breaks on ecl when CC=ccache gcc--obviously this can be worked around easily enough, but setting CC like that is not uncommon either, and only seems to be a problem with ecl.

Would this be worth patching in ecl (both upstream and in sage)?

comment:13 Changed 5 years ago by embray

The updated spkg mentioned in this ticket is a 404 now.

comment:14 Changed 5 years ago by fbissey

Current upstream is https://common-lisp.net/project/ecl/ Also ecl-16.0.0 has been released and it would be nice to upgrade. It would be worth checking if it is still an issue in the latest release.

comment:15 Changed 5 years ago by embray

There is a similar bug that I've just run into, but in Python, when compiling the Cython sources. I've never run into this before strangely, but maybe have just worked on fewer projects with cpp generated from Cython: https://bugs.python.org/issue8027

I will check if the ecl issue is fixed upstream.

Last edited 5 years ago by embray (previous) (diff)

comment:16 Changed 5 years ago by embray

FYI this bug still exists upstream, or at least in ecl-16.0.0. I will see if I can come up with a patch.

comment:17 Changed 5 years ago by embray

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:18 follow-up: Changed 5 years ago by embray

  • Branch set to u/embray/issue-13044
  • Commit set to c9f77c6dc61f6d3684716df620706f05a0604b64
  • Status changed from needs_work to needs_review

Added the patch to the spkg. It applies to the current version of the spkg with a bit of fuzz.

If I understand correctly I should also update the patch version, and upload a new source package? Not sure exactly how to go about that though.


New commits:

c9f77c6Add patch to ecl for building when CC=ccache gcc (or the like) from https://gitlab.com/embeddable-common-lisp/ecl/merge_requests/20

comment:19 Changed 5 years ago by jdemeyer

  • Authors changed from R. Andrew Ohana to R. Andrew Ohana, Erik Bray
  • Description modified (diff)
  • Milestone changed from sage-6.4 to sage-7.2

comment:20 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

Replying to embray:

Added the patch to the spkg. It applies to the current version of the spkg with a bit of fuzz.

If I understand correctly I should also update the patch version

In this exceptional case I think it's not needed because there is no reason to rebuild everybody's ECL with this patch. And in the cases where ECL didn't install before, there is no rebuild needed since there couldn't have been a build in the first place.

and upload a new source package?

No, because you are not changing the source tarball, just adding a patch.

comment:21 Changed 5 years ago by embray

If I understand correctly, for the package the patches are being applied by spkg-src which only seems to apply when building the source tarball.

comment:22 Changed 5 years ago by jdemeyer

Okay, this is unusual...

comment:23 Changed 5 years ago by fbissey

Bad separation (i.e. none) between the patches that needs to be applied in spkg-src because autoconf has to be run after them and "normal" patches. I guess since you are patching the tarball anyway, you may as well put *all* the patches in it.

comment:24 Changed 5 years ago by embray

FWIW the patch has been accepted as-is upstream.

comment:25 Changed 5 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
  • Status changed from needs_review to needs_work

comment:26 Changed 5 years ago by jdemeyer

  • Authors changed from R. Andrew Ohana, Erik Bray to R. Andrew Ohana, Erik Bray, Jeroen Demeyer
  • Reviewers set to Jeroen Demeyer

comment:27 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:28 Changed 5 years ago by jdemeyer

  • Branch changed from u/embray/issue-13044 to u/jdemeyer/issue-13044

comment:29 Changed 5 years ago by jdemeyer

  • Commit changed from c9f77c6dc61f6d3684716df620706f05a0604b64 to 9049b155a9317d421de5ae9c22fad48eca5af824
  • Status changed from needs_work to needs_review

Building with CC="ccache cc" CXX="ccache c++" now works.

I cleaned up the packaging and build of ECL. In particular, I separated the patches which need to be applied by spkg-src from the ones which are applied at build-time. This part still needs to be reviewed.


New commits:

9049b15Improve ECL packaging and build

comment:30 Changed 5 years ago by embray

  • Status changed from needs_review to positive_review

+1

Last edited 5 years ago by embray (previous) (diff)

comment:31 Changed 5 years ago by jdemeyer

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Erik Bray

comment:32 Changed 5 years ago by vbraun

  • Branch changed from u/jdemeyer/issue-13044 to 9049b155a9317d421de5ae9c22fad48eca5af824
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 5 years ago by jdemeyer

  • Commit 9049b155a9317d421de5ae9c22fad48eca5af824 deleted
  • Description modified (diff)
Note: See TracTickets for help on using tickets.