Page MenuHomeFreeBSD

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

Authored by diizzy on Sun, May 12, 1:10 PM.
Tags
None
Referenced Files
F83971747: D45165.diff
Fri, May 17, 3:02 PM
F83943502: D45165.id138432.diff
Fri, May 17, 4:31 AM
F83943501: D45165.id138438.diff
Fri, May 17, 4:31 AM
Unknown Object (File)
Tue, May 14, 8:17 PM
Unknown Object (File)
Tue, May 14, 1:52 PM
Unknown Object (File)
Tue, May 14, 7:57 AM
Unknown Object (File)
Mon, May 13, 7:02 PM
Unknown Object (File)
Mon, May 13, 6:58 PM
Subscribers

Details

Reviewers
arrowd
Group Reviewers
portmgr
Summary

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

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

diizzy created this revision.

Attach patch generated with -U99999 for better context

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 added inline comments.
Mk/Uses/ninja.mk
38–40

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.

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

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

Mk/Uses/ninja.mk
57

Can you put back that whitespace ?

This revision is now accepted and ready to land.Mon, May 13, 5:59 AM
tobik added inline comments.
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

  • Change !empty(ninja_ARGS:M*samurai*) to !empty(ninja_ARGS:Msamurai) as suggested by tobik
  • Fix whitespace as noted by mat
This revision now requires review to proceed.Mon, May 13, 5:47 PM

@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.

Add missing check for MAKE_ARGS

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: ports@FreeBSD.org
 
 .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.

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

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

Mk/Uses/ninja.mk
10
25–31

Add "(implies make)" to description

diizzy added inline comments.
Mk/Uses/ninja.mk
25–31

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

Mk/Uses/ninja.mk
25–31

No, this is simplifying the logic.

Mk/Uses/ninja.mk
25–31

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

Mk/Uses/ninja.mk
25–31

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

Simplify logic as suggested by mat

Mk/Uses/ninja.mk
78

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

USES=   ninja:samurai,run
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
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.

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?

Mk/Uses/ninja.mk
78

Ideas?

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.

Don't safeguard run option

I guess this is good to go now?