Mk/Uses/ninja.mk: Add option to use samurai as ARGS

[diizzy]

Summary

To avoid circular dependencies for example with CMake's dependencies add the ability to specify samurai as an alternative to ninja

[diizzy]

Attach patch generated with -U99999 for better context

[arrowd]

How about having

.if ${NINJA_DEFAULT} == samurai || ${ninja_ARGS:M*samurai*}
_with_samurai= yes
.endif

And then use defined(_with_samurai) in other checks?

[mat]

Mk/Uses/ninja.mk

41–43

I don't think this .if is a good idea, if you leave out, you don't need the .elif later when adding the BuiLD_DEPENDS.

[diizzy]

• Use internal variable instead of evaluating supplied ARGS
• Simplify if statements

[diizzy]

Thanks for the feedback, I think this one is a bit better

[mat]

Mk/Uses/ninja.mk

57

Can you put back that whitespace ?

[tobik]

Mk/Uses/ninja.mk

25

* is unnecessary (we've already checked that ninja_ARGS contains only a known set of words, one of which is samurai).

29

The condition should be just empty(ninja_ARGS:Nsamurai):

• That USES=ninja:samurai implies make makes sense
• However USES=ninja:samurai,build,run should be something that you can do without having MAKE_CMD set just like with USES=ninja:build,run

How about replacing the _SAMURAI_FROM_ARGS thing with

.if ${ninja_ARGS:Msamurai}
_NINJA_SELECT=samurai
.else
_NINJA_SELECT=${NINJA_DEFAULT}
.endif

and s/NINJA_DEFAULT/_NINJA_SELECT/ below

[diizzy]

• Change !empty(ninja_ARGS:M*samurai*) to !empty(ninja_ARGS:Msamurai) as suggested by tobik
• Fix whitespace as noted by mat

[diizzy]

@arrowd @mat
Any other changes you'd like to see or can I push it?

Mk/Uses/ninja.mk

29

.if ${ninja_ARGS:Msamurai} causes Malformed conditional (${ninja_ARGS:Msamurai}) if USES= ninja:samurai isn't defined (took me a while to figure out the cause before submitting a patch)

Given how the framework is structured I'd leave the run option alone for now, the intention is to "temporarily" ignore default settings for building.

We can change the variable name later on if needed as it wont cause any practical changes. As there are multiple ways to define the "backend" I think there's a benefit in having a variable name that clearly shows origin to avoid unnecessary confusion.

[diizzy]

Add missing check for MAKE_ARGS

[tobik]

Mk/Uses/ninja.mk

29

My suggestion wasn't a nitpick for the variable name. I'd never come in here and suggest that. I'd like to clear up what I meant with a diff which is probably clearer than more words. After that I'll leave you alone.

There can't be a malformed conditional since afer the first empty() ninja_ARGS is a) defined (this would already be enough) and b) has at least one word (make).

diff --git a/Mk/Uses/ninja.mk b/Mk/Uses/ninja.mk
index 5c78cf152d07..5c9f2b4f1be1 100644
--- a/Mk/Uses/ninja.mk
+++ b/Mk/Uses/ninja.mk
@@ -7,13 +7,14 @@
 # build			add a build dependency on ninja
 # make			use ninja for the build instead of make, implies "build"
 # run			add a run dependency on ninja
+# samurai		use samurai irregardless of NINJA_DEFAULT
 #
 # MAINTAINER: [e-mail]
 
 .if !defined(_INCLUDE_USES_NINJA_MK)
 _INCLUDE_USES_NINJA_MK=	yes
 
-_valid_ARGS=	build make run
+_valid_ARGS=	build make run samurai
 
 .  for _arg in ${ninja_ARGS}
 .    if empty(_valid_ARGS:M${_arg})
@@ -21,18 +22,24 @@ IGNORE=	'USES+= ninja:${ninja_ARGS}' usage: argument [${_arg}] is not recognized
 .    endif
 .  endfor
 
-.  if empty(ninja_ARGS)
+.  if empty(ninja_ARGS:Nsamurai)
 ninja_ARGS+=	make
 .  endif
 
+.  if ${ninja_ARGS:Msamurai}
+_NINJA_SELECT=	samurai
+.  else
+_NINJA_SELECT=	${NINJA_DEFAULT}
+.  endif
+
 .  if ${ninja_ARGS:Mmake}
 ninja_ARGS+=	build
 .  endif
 
-.  if ${NINJA_DEFAULT} == ninja
+.  if ${_NINJA_SELECT} == ninja
 NINJA_CMD=	ninja
 _NINJA_PORT=	devel/ninja
-.  elif ${NINJA_DEFAULT} == samurai
+.  elif ${_NINJA_SELECT} == samurai
 NINJA_CMD=	samu
 _NINJA_PORT=	devel/samurai
 MAKE_ENV+=	SAMUFLAGS="-v -j${MAKE_JOBS_NUMBER}"
@@ -56,7 +63,7 @@ MAKE_ENV+=	NINJA_STATUS="[%p %s/%t] "
 .  endif
 
 .  if ${ninja_ARGS:Mmake}
-.    if ${NINJA_DEFAULT} == ninja
+.    if ${_NINJA_SELECT} == ninja
 # samu does not support GNU-style args, so we cannot just append
 # -v last.  samu gets this via SAMUFLAGS above but ninja does not
 # support an equivalent environment variable.

[diizzy]

I think we misunderstood each other, either variant is fine by me

...and I should probably mention that I haven't tested your variant

[mat]

Mk/Uses/ninja.mk

10

25–30

[diizzy]

Add "(implies make)" to description

[diizzy]

Mk/Uses/ninja.mk

25–30

I guess this was just to clarify your comment about (implies make)?

[mat]

Mk/Uses/ninja.mk

25–30

No, this is simplifying the logic.

[diizzy]

Mk/Uses/ninja.mk

25–30

Can you post that as a patch file or something as Phab really makes it hard to figure out changes?

[mat]

Mk/Uses/ninja.mk

25–30

You know, you can select the patch, copy it, and your clipboard will contain :

endfor

- .  if !empty(ninja_ARGS:Msamurai)
+ .  if empty(ninja_ARGS)
+ ninja_ARGS+=    make
+ .  elif !empty(ninja_ARGS:Msamurai)
_SAMURAI_FROM_ARGS=     yes
- .  endif
- 
- .  if empty(ninja_ARGS) || defined(_SAMURAI_FROM_ARGS)
ninja_ARGS+=    make
.  endif

.  if ${ninja_ARGS:Mmake

So what I changed your code to be was :

endfor

.  if empty(ninja_ARGS)
ninja_ARGS+=    make
.  elif !empty(ninja_ARGS:Msamurai)
_SAMURAI_FROM_ARGS=     yes
ninja_ARGS+=    make
.  endif

[diizzy]

Simplify logic as suggested by mat

[mat]

Mk/Uses/ninja.mk

78

I'm not sure about this one, what should happen if I write this :

USES=   ninja:samurai,run

[diizzy]

Mk/Uses/ninja.mk

78

How about something like this as we shouldn't allow NINJA_DEFAULT to be completely overriden and by doing so defeating the purpose of NINJA_DEFAULT?

.  elif {ninja_ARGS:Mrun} && defined(_SAMURAI_FROM_ARGS)
IGNORE=		defining runtime dependency to override NINJA_DEFAULT using 'USES+= ninja:samurai,run' is not supported

[mat]

Mk/Uses/ninja.mk

78

I don't understand, why would it not be supported ?
Just remove the part of the !if you added and then it's magically supported.

[diizzy]

Mk/Uses/ninja.mk

78

I'm well aware that it's doable but we might want to safeguard it a bit as it also clashes with NINJA_DEFAULT and might be abused because of it?

[diizzy]

Mk/Uses/ninja.mk

78

Ideas?

[arrowd]

Mk/Uses/ninja.mk

78

I don't think anyone will ever use samurai,run to begin with. But even so, run-depending on samurai in this case perfectly makes sense to me. I'd just remove the && !defined(_SAMURAI_FROM_ARGS) part from the condition and be done with that.

[diizzy]

Don't safeguard run option

[diizzy]

I guess this is good to go now?