Skip to content

Conversation

@msalle
Copy link
Member

@msalle msalle commented Jan 28, 2020

This fixes #118
There are probably a few other places where we should bail out with a non-zero
exit value but this certainly fixes the main ones.

This fixes gridcf#118
There are probably a few other places where we should bail out with a non-zero
exit value.
Since we join the lines via && \ we should only keep the first at sign.
@fscheiner fscheiner self-requested a review February 6, 2020 14:06
Since make automatically returns to the top_srcdir after running, we should not
combine the mkdir and cp with it.
@fscheiner
Copy link
Member

fscheiner commented Feb 13, 2020

@msalle , @matyasselmeci :
I just wondered, if there maybe is a switch for make to cancel further processing of a makefile if a rule (or recipe line) exits with !0? EDIT: E.g. like set -e for shells.
I mean that sounds like a legitimate case and for the "opposite" case - i.e. trying to "make" as many targets as possible to e.g. detect independent problems - there exists -k|--keep-going.

If there isn't a switch for our case, let's pull these changes.

@matyasselmeci
Copy link
Contributor

I'm confused. Exiting when a command returns nonzero is the default behavior for make.

I see why the || exit 1 is needed in the @if [ -f $(@_stamp) ];... line -- the if swallows up the exit code. I don't understand why it's needed in any of the $(MAKE) $(AM_MAKEFLAGS)... lines -- if the $(MAKE) returns nonzero then that line already fails.

@matyasselmeci
Copy link
Contributor

Also, you can add set -e to a recipe line, like

@set -e; export PKG_CONFIG_PATH=$(PKG_CONFIG_PATH) ; cd $($@_SUBDIR) && $(MAKE) $(AM_MAKEFLAGS) install

then the line will fail if the cd fails, not just when the $(MAKE) fails.

@fscheiner
Copy link
Member

What I understood was, that even if a recipe line exits with !0, that does not mean, that make stops processing the remainder of the makefile or rule, e.g. for make install wouldn't it continue with installing other parts of the toolkit, when one part fails installing and just report that error? From the description in #118 I understood it that way.

And the patches should make sure that make stops execution as soon as an error happens IIUC.

@matyasselmeci
Copy link
Contributor

If you're running parallel make (make -j) then if one target fails, it's not going to kill the other targets. I'm not aware of a way around that. Other than kill $$PPID in the recipe line to kill the make process itself. That one sounds kinda nasty though :-)

@fscheiner
Copy link
Member

If you're running parallel make (make -j) then if one target fails, it's not going to kill the other targets. I'm not aware of a way around that.

Ok, that could explain why it doesn't stop right away. #118 explicitly mentions the use of -j 3 (both for make and make install - BTW, is the latter recommended?).

Other than kill $$PPID in the recipe line to kill the make process itself. That one sounds kinda nasty though :-)

So @msalle's patches won't change that behaviour for parallel make runs?

@matyasselmeci
Copy link
Contributor

I wouldn't recommend parallel make for make install. make install is I/O bound anyway (since it's mostly file system operations), so adding -j3 wouldn't give much of a speedup.

And yes, @msalle's patches won't help that case. I think keeping the exit 1s in the if blocks is still useful though.

@msalle
Copy link
Member Author

msalle commented Feb 13, 2020

Hi,
you're probably right about the non-parallel not needing it. The exit 1 does help for a make -j ... though

@matyasselmeci
Copy link
Contributor

Well it works for getting a nonzero error code, but it doesn't interrupt the make. I ran some tests on my laptop (Fedora 31, make 4.2.1).

Here's my Makefile (exittest.mk):

.PHONY: all error ok
all: error ok

error:
	exit 1

ok:
	sleep 5
	echo ok

and here's what happens if I run it:

$ make -j2 -f exittest.mk all; echo exit code $?
exit 1
sleep 5
make: *** [exittest.mk:6: error] Error 1
make: *** Waiting for unfinished jobs....
echo ok
ok
exit code 2

@msalle
Copy link
Member Author

msalle commented Feb 14, 2020

Sure, it won't kill the running parallel jobs (see also https://www.gnu.org/software/make/manual/html_node/Parallel.html), but I think it is useful if at least the outer make process exit with a non-zero exit value if one of its child parallel jobs have failed?
Either way is fine with me, because as you said, make -j... install is generally not ideal, so feel free to close this PR and make a new one if you prefer.

@matyasselmeci
Copy link
Contributor

matyasselmeci commented Feb 14, 2020

I think it is useful if at least the outer make process exit with a non-zero exit value if one of its child parallel jobs have failed?

It already does: the exit status is 2.

@msalle
Copy link
Member Author

msalle commented Feb 14, 2020

Hmm, you have a point there. I'm not sure what originally went wrong, but I agree it's probably indeed not necessary. Perhaps I got confused with the if which indeed would eat up the exit value. The other targets probably exit automatically when its make fails.
I'm afraid I don't have time reasonably soon to fix this and test this properly. If either of you have more time, please go ahead and fix in a better way.

@fscheiner fscheiner merged commit f536970 into gridcf:master Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gsi-openssh silently fails to install if not running as root

3 participants