Makefile fixes

[Djfe]

I formatted the less important prosa as quotes to highlight the more important info

This PR fixes two bugs I encountered while improving our Makefile.
It calls Gluon's Makefile as a submake.

That's kind of relevant for the first commit.

We reused the variable GLUON_TARGETS for limiting what GLUON_TARGET we build (for loop).
All without knowing that this environment variable already exists within Gluon. (or FFMUC did, I'm not sure, we basically copied their nice makefile at some point)
This was fine while MAKE didn't call $(GLUON_MAKE) as a sub make. I fixed this by adding a plus to the shell lines calling another MAKE process.
Since GLUON_TARGET is a variable set on the command line it takes precendence over all occurences of the variable within all makefiles (atleast for make and it's submakes) no matter what the sub makes set.
This behavior can also be reproduced by calling GLUON's Makefile directly. (though noone would do so usually since the variable isn't supposed to be set, but we set it unintentionally)

My fix:
Rename GLUON_TARGETS to GLUON_TARGETLIST
use "override" when setting GLUON_TARGETS
This way the new values always take precedence regardless of what users input.
The first occurence of GLUON_TARGETS := actually resets it to the empty string. Now it also/actually empties it when set on the command line

This mostly prevents confusing output like this, it didn't cause any actual bugs on our end:

make download all GLUON_TARGETS="DEADBEEF" GLUON_SITEDIR="contrib/ci/minimal-site"
Please set GLUON_TARGET to a valid target. Gluon supports the following targets:
 * DEADBEEF
make: *** [Makefile:176: config] Error 1

(when leaving BROKEN unset, intentionally, to provoke the error message for an existing target)

make download all GLUON_TARGETS="bcm27xx-bcm2710" GLUON_SITEDIR="contrib/ci/minimal-site"
Please set GLUON_TARGET to a valid target. Gluon supports the following targets:
 * bcm27xx-bcm2710
make: *** [Makefile:176: config] Error 1

---

Second commit:
I stumbled across the weird way Gluon handles the environment variable BROKEN:
first of, here are the definitions
devices
targets

why weird you ask? Well, it's inconsistent.

broken Devices get build, when BROKEN is evaluated to true (by Lua):
true: 1, 0.1, 0.9, 2, 1000
false: 0, -1, -4.5, a, b, bullshit
true meaning: any positive non-zero integer or float

broken Targets on the other hand, they get build, when BROKEN exists as a (non-empty) variable.
true is anything from: bullshit, 0, -4.5, word, 1, 0.5, etc.

I wanted to be able to set BROKEN=0
A because our default is 1, B because BROKEN="" is unintuitive for users
a simple export BROKEN ?= 1 now works fine with my fix

Without the fix, I needed to filter the variable MAKEOVERRIDE to remove BROKEN=0 (or set it to an empty string, I only realized this during writing the PR)
and I need to unexport BROKEN as well
Better make the behavior consistent, so here's a fix :)

The fix isn't perfect, but it catches the relevant numbers (0 and 1, as well as an empty or unset variable), so it should be fine.
new behavior:
devices stays the same
targets can't (easily) compare numbers (or floats for that matter): only an exact "1" builds BROKEN devices, everything else does nothing

I could adjust Devices so it matches Targets behavior exactly, the decision is up to you
I could also add documentation for BROKEN (I think there is none, yet?)

[neocturne]

Hmm, I'm not entirely happy with the solution. By the same logic that applies to GLUON_TARGETS, there are a bunch of other variables that might have to be overridden if set from the outside. make is inherently fragile in this way, and I don't think there is much value in trying to work around the issue, rather than just telling people not to set variables on the command line that are not explicitly allowed by our docs.

Also, your commits need better descriptions for what they fix directly in the commit message. If we ever decide to ditch Github, PR descriptions will be lost, but commit messages won't.

[Djfe]

There's not many other variables users could override by accident. 5 or so, I think. This one has the closest name of an actual variable (GLUON_TARGET)
What about renaming GLUON_TARGETS? (it's just a temporary variable, right?)
The users of our and ffmuc's makefile are used to setting this variable now. The name was derived from GLUON_TARGET but our makefile wants to allow building more than one (successively).
ffmuc doesn't have this issue because they don't call Gluon as a submake, yet. (they call it as a normal shell line)

not really a gripe of mine but by your rule Noone should be setting BROKEN either hehe (never mentioned in the docs iirc). not that this matters.

Also, your commits need better descriptions for what they fix directly in the commit message. If we ever decide to ditch Github, PR descriptions will be lost, but commit messages won't.

that's new to me. Sure I could do that. Are you talking about commit titles only or actual commit descriptions?
@AiyionPrime told me to avoid commit descriptions since Noone did them. also it keeps the git log shorter/clearer

[neocturne]

There's not many other variables users could override by accident. 5 or so, I think. This one has the closest name of an actual variable (GLUON_TARGET) What about renaming GLUON_TARGETS? (it's just a temporary variable, right?) The users of our and ffmuc's makefile are used to setting this variable now. The name was derived from GLUON_TARGET but our makefile wants to allow building more than one (successively). ffmuc doesn't have this issue because they don't call Gluon as a submake, yet. (they call it as a normal shell line)

Renaming the variable would be fine.

not really a gripe of mine but by your rule Noone should be setting BROKEN either hehe (never mentioned in the docs iirc). not that this matters.

I think it makes sense to mention the variable somewhere in the docs, at least in the "development" section.

Also, your commits need better descriptions for what they fix directly in the commit message. If we ever decide to ditch Github, PR descriptions will be lost, but commit messages won't.

that's new to me. Sure I could do that. Are you talking about commit titles only or actual commit descriptions? @AiyionPrime told me to avoid commit descriptions since Noone did them. also it keeps the git log shorter/clearer

My opinion is the opposite. Commit descriptions should contains a full explanation of the whats and whys if it's not obvious from the change itself (also keep in mind that what may be obvious now may not be when someone tried to find the reason for a change in a few years). For some commits the title is enough ("add device" or "update module" is usually sufficiently self-explanatory), but often having some additional explanation is preferable.

PR descriptions can just be a copy of the commit description then (which Github will already do by default for single-commit PRs), or summarize the overall changes on a higher level if the PR contains multiple related commits.

[Djfe]

I just updated my PR and added a commit description to both commits. Feel free to review :)

[neocturne]

Commit descriptions are looking good now, but you don't need to include the PR number - for squashes, this will be added by Github, and for merges, the merge commit contains the PR number.

I don't like the variable name GLUON_TARGETLIST particularly... maybe all internal variables could be changed to use something like _GLUON_ as their prefix? It's still wrong for sites to pass any GLUON_ variables that aren't mentioned in the docs into Gluon's make, but it should make the Makefile a bit more robust against such mistakes.

[Djfe]

I dropped the commit editing the variable name
any objections to also backport the remaining commit that changes the usage of BROKEN?

[github-actions]

Successfully created backport PR for v2023.1.x:

• [Backport v2023.1.x] Makefile fixes #3103