Closed Bug 450948 Opened 15 years ago Closed 15 years ago

Remove Mac case from AUTOCONF detection code.


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: romaxa, Assigned: kairo)




(2 files, 2 obsolete files)

Attached patch Bugfix (obsolete) — Splinter Review
Please check it on Mac OSX environment...
Attachment #334168 - Flags: review?(ted.mielczarek)
Blocks: 450967
Comment on attachment 334168 [details] [diff] [review]

Looks ok to me.
Attachment #334168 - Flags: review?(ted.mielczarek) → review+
Ok, this breaks heavily. I don't think it's exactly what we want.

cb-sea-miniosx01:~ seabld$ which autoconf-2.13 autoconf2.13 autoconf213 | head -1
no autoconf-2.13 in /opt/local/bin /opt/local/sbin /bin /sbin /usr/bin /usr/sbin

A basically working version is:

which autoconf-2.13 autoconf2.13 autoconf213 2>&1 | grep -v 'no autoconf' | grep -v 'unknown command' | head -n1

but this sound like it easily might break again.

Why it needs to be that way:
1) mac sends an error like "no autoconf-2.13 in ..." to stdout
2) linux sends an error like "which: no autoconf213 in ..." to stderr
3) Windows sends an error like "which: autoconf2.13: unknown command"

I wonder if we should completely revert all this.
actually, the 1) case is 10.4 only, 10.5 sends to stderr like it should.

So this would work as well:

# discard errors from 'which'
# MacOS X 10.4 sends "no autoconf*" errors to stdout, discard those via grep
AUTOCONF ?= $(shell which autoconf-2.13 autoconf2.13 autoconf213 2>/dev/null | grep -v 'no autoconf' | head -1)
Instead of relying on which, and effectively on the filename, why not just invoke the various possible command names (trying plain 'autoconf' last) as:

$> $cmd --version

And picking the first one that correctly reports being autoconf 2.13
Feels like overkill, especially since this will be executed every time is run. I'd be fine with Kairo's suggestion there.
Attached patch simple, slightly hacky fix (obsolete) — Splinter Review
Here's the fix I proposed in the comment in patch form. It's simple, but slightly hacky, mainly because of the which bug on Tiger.
Assignee: nobody → kairo
Attachment #334168 - Attachment is obsolete: true
Attachment #334473 - Flags: review?(ted.mielczarek)
Comment on attachment 334473 [details] [diff] [review]
simple, slightly hacky fix

Can you make that "grep -v '^no autoconf'" ?
Attachment #334473 - Flags: review?(ted.mielczarek) → review+
change to "grep -v '^no autoconf'" as suggested in comment #7
Attachment #334473 - Attachment is obsolete: true
Attachment #334892 - Flags: review?(ted.mielczarek)
gozer, he already has reviewed it with this patch, so I'll check it in that way, but I'm waiting for the tree to reopen ;-)
Comment on attachment 334892 [details] [diff] [review]
simple, slightly hacky fix

Yeah, you didn't really need to request review again, but here it is anyway.
Attachment #334892 - Flags: review?(ted.mielczarek) → review+
Thanks, pushed as - I hope this version sticks.
Closed: 15 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.