error on spaces in XPI_NAME and some other Makefile vars

RESOLVED FIXED

Status

defect
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: ted, Assigned: ted)

Tracking

Trunk

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

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
Posted patch strip XPI_NAME (obsolete) — Splinter Review
I didn't test this, but it's pretty simple.  We might even consider erroring if XPI_NAME is set but contains spaces.
Attachment #255988 - Flags: review?(benjamin)
(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

12 years ago
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 ;-)
Attachment #255988 - Flags: review?(benjamin) → review-
(Assignee)

Updated

12 years ago
Summary: strip trailing spaces from XPI_NAME when setting FINAL_TARGET → warn on trailing spaces in XPI_NAME (breaks FINAL_TARGET)
This checks XPI_NAME for internal or trailing spaces.  Are there other vars you think we should sanity check while I'm doing this?
Attachment #255988 - Attachment is obsolete: true
Attachment #256032 - Flags: review?(benjamin)

Comment 5

12 years ago
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.
Attachment #256032 - Flags: review?(benjamin) → review-
Okay, this checks all the vars that bsmedberg mentioned, and uses a slightly modified version of his filter-out example to do so.
Attachment #256032 - Attachment is obsolete: true
Attachment #259684 - Flags: review?(benjamin)

Comment 7

12 years ago
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.
Attachment #259684 - Flags: review?(benjamin) → review+
Checked in with that fix.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Summary: warn on trailing spaces in XPI_NAME (breaks FINAL_TARGET) → error on spaces in XPI_NAME and some other Makefile vars

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.