make: fix possible PATH issues

[benoit-pierre]

Ensure our possibly updated PATH variable is taken into account for all $(shell …) invocations.

Additionally, add a couple of sanity checks after some of those invocations). Nope, still sloppy…

---

This change is 

[NiLuJe]

I have some make nastiness for that, FWIW.

(Probably ripped inspired from Linux's makefiles, IIRC).

[NiLuJe]

I like it (assuming it works ;p).

[benoit-pierre]

Oh, boy… I'm afraid to look at those logs.

[benoit-pierre]

Of course, can't check if one those $(shell …) invocations was successful because we may not be building and need those variables…

[benoit-pierre]

Sloppy it is, then.

[NiLuJe]

Re-reading the docs (without testing anything so far), I'm mildly confused by all those existing shell + PATH hacks: the shell function is already supposed to pickup the env (and exported variables), and the only thing that actually updates PATH inside the makefiles is the Android stuff (... at least in base, the front one doesn't use export, which is either an oversight or I'm on not on the latest code).

[benoit-pierre]

I know! ¯\(ツ)/¯

But I can reproduce it when running the android docker image locally…

[NiLuJe]

(... at least in base, the front one doesn't use export, which is either an oversight or I'm on not on the latest code).

Yeah, the front one was broken before, but is entirely gone after koreader/koreader@3ac8a67, and it's required by the MACHINE assignment, which is how you end up with a broken tuple ;).

[NiLuJe]

But I can reproduce it when running the android docker image locally…

Is the Android TC perhaps already in your PATH in that environment?

[NiLuJe]

Is the Android TC perhaps already in your PATH in that environment?

Which would explain https://github.com/koreader/koreader/blob/3ac8a67c6d39ce09f0cdbdfface63498857a6f22/Makefile#L34 successfully picking up the compiler on your end and not in the CI?

[benoit-pierre]

But I can reproduce it when running the android docker image locally…

Is the Android TC perhaps already in your PATH in that environment?

Nope.

And no .SHELLSTATUS either, by the way. Maybe because the make version (4.1) is too old?

[NiLuJe]

Oh, wait, I misread

But I can reproduce it when running the android docker image locally…

as cannot reproduce, oops. :D.

[benoit-pierre]

And when compiling with my local environment, the android toolchain is not in the PATH either, but it works fine, maybe, again, because the make version is newer.

[NiLuJe]

And no .SHELLSTATUS either, by the way. Maybe because the make version (4.1) is too old?

A (very) quick SO search seems to imply it's 4.2+, yeah :/.

(.SHELLSTATUS support, I mean).

[benoit-pierre]

Yeah, I hate that the make documentation does not mention the version that first introduced a feature…

[NiLuJe]

If all else fails, I'd try removing all the explicit PATH= hacks inside shell calls (and hope make is actually working as advertised on that front), but restoring an export PATH assignment in an Android branch in front ahead of the MACHINE assignment.

[NiLuJe]

And when compiling with my local environment, the android toolchain is not in the PATH either

Extremely sneaky but stupid idea: do you have a PATH variable set in your environment (like, at all)?

[benoit-pierre]

But we already do set PATH before (around line 147) the call to set TARGET_MACHINE (around line 559).

[benoit-pierre]

And when compiling with my local environment, the android toolchain is not in the PATH either

Extremely sneaky but stupid idea: do you have a PATH variable set in your environment (like, at all)?

Yes.

[NiLuJe]

But we already do set PATH before (around line 147) the call to set TARGET_MACHINE (around line 559).

Which makefile are we talking about here? ^^

And when compiling with my local environment, the android toolchain is not in the PATH either

Extremely sneaky but stupid idea: do you have a PATH variable set in your environment (like, at all)?

Yes.

Does it work with PATH entirely unset and gone from the env?

[benoit-pierre]

But we already do set PATH before (around line 147) the call to set TARGET_MACHINE (around line 559).

Which makefile are we talking about here? ^^

base/Makefile.defs.

And when compiling with my local environment, the android toolchain is not in the PATH either

Extremely sneaky but stupid idea: do you have a PATH variable set in your environment (like, at all)?

Yes.

Does it work with PATH entirely unset?

I've not checked, but the nightly build script does set PATH.

[NiLuJe]

Possibly related (or not, I can never grok how the fuck these assignments differ in practice), but MACHINE uses =, while most everything else uses := (because that one is less awful, in my experience ;D).

[NiLuJe]

I've not checked, but the nightly build script does set PATH.

I don't recall if the front CI does, though.

[benoit-pierre]

From TARGET_MARCHINE. :P

[NiLuJe]

But we already do set PATH before (around line 147) the call to set TARGET_MACHINE (around line 559).

Which makefile are we talking about here? ^^

base/Makefile.defs.

Oh, true, but I have an inkling it might the front one failing: https://github.com/koreader/koreader/blob/3ac8a67c6d39ce09f0cdbdfface63498857a6f22/Makefile#L34

(And I'm not sure if it isn't actually assigned before the base includes (with its proper PATH export) gets processed, because make is weird or something).

[benoit-pierre]

But we already do set PATH before (around line 147) the call to set TARGET_MACHINE (around line 559).

Which makefile are we talking about here? ^^

base/Makefile.defs.

Oh, true, but I have an inkling it might the front one failing: https://github.com/koreader/koreader/blob/3ac8a67c6d39ce09f0cdbdfface63498857a6f22/Makefile#L34

Why are we duplicating those variables?

It's still done after including base/Makefile.defs, so after setting PATH.

[NiLuJe]

Why are we duplicating those variables?

It's still done after including base/Makefile.defs, so after setting PATH.

Yeah, see my edits above, that's what I'm not sure on because of the seven billion ways there are to assign variables in make, I'm not sure the sensible thing is actually what's happening here...

[benoit-pierre]

Since it's set with =, it's only going to be expanded on use, so even later, so is something messing with PATH / CC before that?

[NiLuJe]

We can't guarantee that base will be checked out yet at the time the fetchthirdparty target is called, so we don't necessarily have those defines yet?

That would both explain the weird duplication, and the silently-failing include (-include).

[NiLuJe]

(I may have lost the plot a little, it's 2AM, sorry if I'm making less and less sense ;p)

[benoit-pierre]

It's definitively an issue with older make versions:

▸ env PATH=/bin:/usr/bin make41 -f - <<\EOF
export PATH=/pouet:/bin:/usr/bin
$(error $(shell echo "$$PATH"))
EOF
/tmp/Gm4xFbl8:2: *** /bin:/usr/bin.  Stop.
▸ env PATH=/bin:/usr/bin make43 -f - <<\EOF
export PATH=/pouet:/bin:/usr/bin
$(error $(shell echo "$$PATH"))
EOF
/tmp/Gm3Vvgp6:2: *** /bin:/usr/bin.  Stop.
▸ env PATH=/bin:/usr/bin make44 -f - <<\EOF
export PATH=/pouet:/bin:/usr/bin
$(error $(shell echo "$$PATH"))
EOF
/tmp/GmGMUP9D:2: *** /pouet:/bin:/usr/bin.  Stop.

[benoit-pierre]

Let me amend the code / commit message to mention that.

[benoit-pierre]

OK, I think this can be merged. And then bump base again, and… wait for the next nightly?

[NiLuJe]

I can't find any mention of the behavior change in the NEWS file, FWIW, yaaay -_-".

[NiLuJe]

I can't find any mention of the behavior change in the NEWS file, FWIW, yaaay -_-".

Actually, I can, it sure helps if I look at the right version xD.

* WARNING: Backward-incompatibility!
  Previously makefile variables marked as export were not exported to commands
  started by the $(shell ...) function.  Now, all exported variables are
  exported to $(shell ...).  If this leads to recursion during expansion, then
  for backward-compatibility the value from the original environment is used.
  To detect this change search for 'shell-export' in the .FEATURES variable.

(In 4.4)

[Frenzie]

And when compiling with my local environment, the android toolchain is not in the PATH either, but it works fine, maybe, again, because the make version is newer.

I can and afaik only 4.4 is newer.

$ make --version
GNU Make 4.3
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Edit: sorry, commented too fast. I see you figured that out too.