371201 - error on spaces in XPI_NAME and some other Makefile vars

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

This can cause us to break with unhelpful error messages like:

/bin/sh: ../../dist/xpi-stage/gban: File exists
make[1]: *** [libs] Error 1
make: *** [all] Error 2

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: strip XPI_NAME

I didn't test this, but it's pretty simple.  We might even consider erroring if XPI_NAME is set but contains spaces.

Comment 2

[David Humphrey (:humph)]

(In reply to comment #1)
> Created an attachment (id=255988) [details]
> strip XPI_NAME
> 
> I didn't test this, but it's pretty simple.  We might even consider erroring if
> XPI_NAME is set but contains spaces.
> 
I've tested this and it works.

Comment 3

[Benjamin Smedberg]

Comment on attachment 255988 [details] [diff] [review]
strip XPI_NAME

After discussion on IRC, we're going to try and create a meaningful error if various important variables have extraneous spaces. This might be hard ;-)

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: warn if spaces or trailing spaces in XPI_NAME

This checks XPI_NAME for internal or trailing spaces.  Are there other vars you think we should sanity check while I'm doing this?

Comment 5

[Benjamin Smedberg]

Comment on attachment 256032 [details] [diff] [review]
warn if spaces or trailing spaces in XPI_NAME

LIBRARY_NAME
MODULE
DEPTH
SHORT_LIBNAME
XPI_PKGNAME
INSTALL_EXTENSION_ID

You can probably shorten this to:

$(if $(filter-out 0 1,$(words $(XPI_NAME))),$(error Spaces are not allowed in XPI_NAME))

And then you could do a $(foreach) to do that for a bunch of variables. Though... that's not pretty.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: check a bunch of variables

Okay, this checks all the vars that bsmedberg mentioned, and uses a slightly modified version of his filter-out example to do so.

Comment 7

[Benjamin Smedberg]

Comment on attachment 259684 [details] [diff] [review]
check a bunch of variables

>Index: config/config.mk

>+CHECK_VARS := XPI_NAME \
>+              LIBRARY_NAME \

I think we should standardize on a whitespace strategy, and I'd prefer

SETVAR = \
  THING1 \
  THING2 \
  $(NULL)

We would only use spaces where absolutely required by rule syntax, and if we change the name of a variable we don't end up changing the spacing.

Other than that, r=me. Thanks.

Comment 8

[(not currently active) Ted Mielczarek]

Checked in with that fix.