Opened 6 years ago

Closed 5 years ago

#22116 closed defect (wontfix)

installation of gap_packages fails with bash 4.4

Reported by: Dima Pasechnik Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: optional Keywords:
Cc: Merged in:
Authors: François Bissey Reviewers: Dima Pasechnik
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: u/fbissey/leon (Commits, GitHub, GitLab) Commit: d3ae65082dc0bfc5a2f240e77882632cbef8e8d1
Dependencies: Stopgaps:

Status badges

Description (last modified by Dima Pasechnik)

The symptom is the error at the line cd leon make coming from a Makefile in guava package.

make[3]: Entering directory '/home/dima/Sage/sage-dev/local/gap/gap-4.8.3/pkg/guava-3.13/src'
gcc  -O2 -L/home/dima/Sage/sage-dev/local/lib -Wl,-rpath,/home/dima/Sage/sage-dev/local/lib  -o leonconv leonconv.c
cd leon make
/bin/sh: line 0: cd: too many arguments
make[3]: *** [Makefile:14: all] Error 1

This happens when /bin/sh is bash 4.4.5(1). New bash versions like this one do not silently ignore extra arguments to cd (even though it is documented as ignored). So the line cd leon make behaves like cd leon which is a no-op since no other commands follow that cd command.

Guava readme on its homepage lists Leon's programs as a part, so they should be built, in fact.

See also the corresponding gentoo github thread.

Change History (32)

comment:1 Changed 6 years ago by François Bissey

Like I said on github I have no idea why it works in the first place on most system. The code is bad.

comment:2 Changed 6 years ago by François Bissey

Authors: François Bissey
Branch: u/fbissey/leon
Commit: abb06b5ba62e5f9c838804b3312d023a99972099
Status: newneeds_review

As, also mentioned in github, my own gentoo systems are quite unaffected by the bug - one day I may understand why that is. In any case, can you pass it upstream Dima?


New commits:

abb06b5Fix guava's src/Makefile for #22116

comment:3 Changed 6 years ago by François Bissey

Hum... Apart from not doing exactly the same thing as in sage-on-gentoo (not woken yet may be) there is the issue that autoreconf is called in src/leon. Do you see that on other systems? It fails miserably in any case but that's not normal or wanted.

comment:4 in reply to:  3 Changed 6 years ago by Dima Pasechnik

Replying to fbissey:

Hum... Apart from not doing exactly the same thing as in sage-on-gentoo (not woken yet may be) there is the issue that autoreconf is called in src/leon. Do you see that on other systems? It fails miserably in any case but that's not normal or wanted.

I don't understand how your patch would make it work; there is no makefile in src/leon/

comment:5 Changed 6 years ago by Dima Pasechnik

IMHO it should be

-       cd leon make
+       cd leon && ./configure && $(MAKE)

comment:6 Changed 6 years ago by François Bissey

The build system is a right mess.

SRCDIR  = ./src/leon
CJSRCDIR= ./src/ctjhai
BINDIR = bin/$(GAPARCH)
#GAP_PATH=../..
#PKG_PATH=.
#SRCDISTFILE=guava

targets: default

default: bindir minimum-weight leonconv desauto install

# this target creates a bin-directory
bindir:
	if test ! -d $(BINDIR);  then mkdir -p $(BINDIR);  fi

minimum-weight: $(CJSRCDIR)/minimum-weight.o $(CJSRCDIR)/minimum-weight-gf2.o $(CJSRCDIR)/minimum-weight-gf3.o $(CJSRCDIR)/popcount.o
	$(CC) $(LDFLAGS) -o $(CJSRCDIR)/minimum-weight \
$(CJSRCDIR)/minimum-weight.o $(CJSRCDIR)/minimum-weight-gf2.o \
$(CJSRCDIR)/minimum-weight-gf3.o $(CJSRCDIR)/popcount.o -lm

leonconv: 
	cd ./src; $(MAKE) CC="$(CC)" CPPFLAGS="$(CPPFLAGS)" CFLAGS="$(CFLAGS)" LDFLAGS="$(LDFLAGS)";

desauto: 
	cd $(SRCDIR); autoreconf --install --force || true ; ./configure; $(MAKE) CC="$(CC)" CPPFLAGS="$(CPPFLAGS)" CFLAGS="$(CFLAGS)" LDFLAGS="$(LDFLAGS)";

so the desauto target will produce a makefile in src/leon and run make. In effect that little bit in src/Makefile would be "useful" if the desauto target was run before leonconv. As it is, it indeed does nothing.

comment:7 Changed 6 years ago by Dima Pasechnik

It probably fails for me due to a race condition (with make -jX with X>>1).

comment:8 Changed 6 years ago by Jeroen Demeyer

This should be reported upstream

comment:9 Changed 6 years ago by François Bissey

Indeed you could fail because you try to run make while it is being produced. That makes the bit of my gentoo patch where I make the leoconv target depend on desauto the real resolution. I think the dependencies of the install target should be fixed as I did in Gentoo as well.

comment:10 Changed 6 years ago by Jeroen Demeyer

Which version of bash are you using?

With

GNU bash, version 4.2.49(1)-release (x86_64-pc-linux-gnu)

it seems that extra arguments to cd are silently ignored.

comment:11 in reply to:  10 Changed 6 years ago by Dima Pasechnik

Replying to jdemeyer:

Which version of bash are you using?

With

GNU bash, version 4.2.49(1)-release (x86_64-pc-linux-gnu)

it seems that extra arguments to cd are silently ignored.

I'm on bash 4.4.5(1) (according to manpages of bash, the args of cd after dir are ignored). If I merely use the branch I still cannot build, I need to run configure in src/leon first. I need to check that this can be replaced by adding the explicit dependency (but I guess it will do).

I'm experimenting with stock GAP 4.8.6 now, same issues.

Last edited 6 years ago by Dima Pasechnik (previous) (diff)

comment:12 Changed 6 years ago by François Bissey

And I am on bash 4.3 myself. It could indeed be down to behavior depending on the bash version, and once you fix the problem with src/Makefile we need to fix the dependencies.

comment:13 Changed 6 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Given that the line cd leon make is essentially a no-op (it behaves like cd leon), maybe it should be removed?

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:14 in reply to:  13 ; Changed 6 years ago by Dima Pasechnik

Replying to jdemeyer:

Given that the line cd leon make is essentially a no-op (it behaves like cd leon), maybe it should be removed?

this is not the original intention, of course. Although it's very old code there.

comment:15 Changed 6 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: installation of gap_packages fails on some gentoo systemsinstallation of gap_packages fails with bash 4.4

comment:16 in reply to:  14 Changed 6 years ago by Jeroen Demeyer

Replying to dimpase:

this is not the original intention, of course. Although it's very old code there.

Given that guava has worked for years with that broken line, I think it's best to keep the behaviour (i.e. doing nothing).

comment:17 in reply to:  14 Changed 6 years ago by François Bissey

Replying to dimpase:

Replying to jdemeyer:

Given that the line cd leon make is essentially a no-op (it behaves like cd leon), maybe it should be removed?

this is not the original intention, of course. Although it's very old code there.

Very organic. Lots of stuff is decrepit and not used. So excision, as suggested by Jeroen, is warranted.

comment:18 Changed 6 years ago by Dima Pasechnik

No, it must be fixed (upstream as well) and preserved. It is working code.

comment:19 Changed 6 years ago by Dima Pasechnik

Description: modified (diff)
Report Upstream: N/ANot yet reported upstream; Will do shortly.

comment:20 Changed 6 years ago by git

Commit: abb06b5ba62e5f9c838804b3312d023a99972099d3ae65082dc0bfc5a2f240e77882632cbef8e8d1

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

d3ae650improve patch to make sure parallel make work as well as not fail with bash 4.4

comment:21 Changed 6 years ago by François Bissey

Status: needs_workneeds_review

This is closer to what I do in Gentoo and make sure it will work properly with parallel make.

comment:22 Changed 6 years ago by Jeroen Demeyer

I would only accept this patch if upstream accepts it.

I would accept removing the line cd leon make (with no further changes) unconditionally.

Last edited 6 years ago by Jeroen Demeyer (previous) (diff)

comment:23 in reply to:  22 Changed 6 years ago by Dima Pasechnik

Replying to jdemeyer:

I would only accept this patch if upstream accepts it.

I am pretty much OK with accepting it, as this seems to be a general GAP(packages) slackness with not fixing obvious things for years.

comment:24 in reply to:  22 ; Changed 6 years ago by François Bissey

Replying to jdemeyer:

I would only accept this patch if upstream accepts it.

I would accept removing the line cd leon make (with no further changes) unconditionally.

Considering the cruft, I would suggest more invasive changes upstream. src/Makefile is really full of unused stuff beside that line. Once it is done the target dependencies in the top makefile could be pushed to the install target only. And finally could we get rid of running autoreconf (note the artful way things are handled on line 29 of Makefile.in so that absence of autoreconf or its failure is not a problem - and because there is an automake statement in src/leon/configure.ac and no Makefile.am, autoreconf will fail if run).

comment:25 in reply to:  24 ; Changed 6 years ago by Jeroen Demeyer

Replying to fbissey:

Considering the cruft, I would suggest more invasive changes upstream.

I don't care.

For Sage for now, why not do the minimal change which is needed to fix the problem with bash 4.4? We can leave further changes for the future, after discussing with upstream.

comment:26 in reply to:  25 Changed 6 years ago by François Bissey

Replying to jdemeyer:

Replying to fbissey:

Considering the cruft, I would suggest more invasive changes upstream.

I don't care.

For Sage for now, why not do the minimal change which is needed to fix the problem with bash 4.4? We can leave further changes for the future, after discussing with upstream.

Sure. My only other changes on that branch are considerations that parallel make will fail for someone sooner or later.

comment:27 Changed 6 years ago by Dima Pasechnik

Milestone: sage-7.5sage-duplicate/invalid/wontfix
Reviewers: Jeroen Demeyer, Dima Pasechnik

I've put the patch into the branch on #20914. Thanks :-)

comment:28 Changed 6 years ago by Dima Pasechnik

Status: needs_reviewpositive_review

comment:29 Changed 6 years ago by Jeroen Demeyer

For the record: I disagree with that patch (but if other people are happy with it, I will not object).

comment:30 Changed 6 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer, Dima PasechnikDima Pasechnik

I think that this bug is important enough that it should be fixed independently of #20914. However, again, I don't want to interfere with what you guys are doing...

comment:31 Changed 6 years ago by Dima Pasechnik

Report Upstream: Not yet reported upstream; Will do shortly.Fixed upstream, in a later stable release.

comment:32 Changed 5 years ago by Erik Bray

Resolution: wontfix
Status: positive_reviewclosed

Since this is duplicate/invalid/wontfix I guess it can be closed?

Note: See TracTickets for help on using tickets.