255328 - mkdepend command line defines may be broken

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Justin Wood (:Callek)]

When running mkdepend if a define is specified as |-D Foo| or |-D Foo=bar|
mkdepend will miss the first two letters of the define when storing it, where-as
normal |-DFoo| and |-DFoo=bar|  will work just fine.

Similar two letter issue is with the -U switch.

Comment 1

[Justin Wood (:Callek)]

Attachment: Fix

adds a register char `offset` to serve as an entry to pass instead of the
unconditionally passed |+ 2|

used register char to keep speed of mkdepend up as much as possible.

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 155884 [details] [diff] [review]
Fix

Bryner, was suggested I r? you.

Do we need sr here, and if so, suggest anyone?

Comment 3

[Justin Wood (:Callek)]

Attachment: Patch v2

Noticed erroneous offset reference (and incorrect at that) called in first
file, that I did not catch my first upload.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 155891 [details] [diff] [review]
Patch v2

porting r?

Comment 5

[Justin Wood (:Callek)]

bsmedberg

Can I get you to peek at this patch and r+ it and comit for me, bryner seems to
be a bit busy as of late.

Comment 6

[Brian Ryner (not reading)]

Does this situation come up somewhere in our build?

Comment 7

[Justin Wood (:Callek)]

I dont believe it does, (I'm not as up on the makefile rules for every configure
format), but I am sure if it did we would have heard of it before now.

I opted for "fix" rather than remove, but if it is your concept to remove the
support for the space then I can do that as well.  I have been running with this
patch for a while, with no real (noticeable) problems, unfortunately I never did
any build time compares.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 155891 [details] [diff] [review]
Patch v2

What I don't like about this is that it creates an inconsistency with the way
other options, like -I, are handled.  So I'd say the either need to all accept
the extra space or none of them... since the problem doesn't come up in our
build I think I'd rather just leave it as-is.

Comment 9

[Justin Wood (:Callek)]

quoting from mkdepend code:

>		case 'I':
>			if (incp >= includedirs + MAXDIRS)
>			    fatalerr("Too many -I flags.\n");
>			*incp++ = argv[0]+2;
>			if (**(incp-1) == '\0') {
>				*(incp-1) = *(++argv);
>				argc--;
>			}
>			break;

Is the section on includes, following that, it does check, and work if there is
a space in the command line |-I somefile.ext|  works as does |-Isomefile.ext| 
in the same way.

The -D issue is not an issue with out default builds, but if someone adds a -D
option themselves to their build process, they would expect mkdepend to work as
expected when determining dependancies...where currently if they do it with a
space they cannot.

With the way the code *looks* right now, it would make people who look at a
glance think that we support it that way, I'm for supporting it, but if we must
leave it out, or remove it, we need to make other argument accepting things with
mkdepend NOT work with a space, such as |-I|

Thoughts?

Comment 10

[Justin Wood (:Callek)]

patch r+'ed based on talk in irc, thanks bryner

Patch checked in thanks to timeless