| Summary: | Fails to build on arm (debian) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mike Hommey <mh+webkit> | ||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ddkilzer, mrowe | ||||||
| Priority: | P2 | ||||||||
| Version: | 523.x (Safari 3) | ||||||||
| Hardware: | Other | ||||||||
| OS: | Linux | ||||||||
| Attachments: |
|
||||||||
|
Description
Mike Hommey
2007-08-26 00:49:15 PDT
Created attachment 16121 [details]
Patch
Adds a test on __arm__
Comment on attachment 16121 [details] Patch Thanks for the patch! A couple of comments. +#if defined(arm) \ + || defined(__arm__) You should just put these on the same line: #if defined(arm) || defined(__arm__) You'll need to create a ChangeLog entry using the prepare-ChangeLog. See <http://webkit.org/coding/contributing.html>. You should also set the review flag to ? when uploading patches so that people know they need review. Once you have a patch with a ChangeLog entry, upload it here and I'll r+ it. (In reply to comment #2) > (From update of attachment 16121 [details] [edit]) > Thanks for the patch! A couple of comments. > > +#if defined(arm) \ > + || defined(__arm__) > > You should just put these on the same line: > > #if defined(arm) || defined(__arm__) Well, I would have, if surrounding #if's weren't split over several lines. So please tell me which way you really prefer ;) > You'll need to create a ChangeLog entry using the prepare-ChangeLog. See > <http://webkit.org/coding/contributing.html>. > > You should also set the review flag to ? when uploading patches so that people > know they need review. Being used to mozilla's bugzilla, I assumed I needed a name for a reviewer, which is why i didn't put the review flag. Created attachment 16122 [details]
Patch v2
This includes proper Changelog. Please tell me for the #if (cf. previous comment)
What is "FTBFS" that you mention in your ChangeLog? The patch looks fine to me except for that cryptic message. (In reply to comment #5) > What is "FTBFS" that you mention in your ChangeLog? The patch looks fine to me > except for that cryptic message. Failed To Build From Source. It's a Debian thing. (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 16121 [details] [edit] [edit]) > > Thanks for the patch! A couple of comments. > > > > +#if defined(arm) \ > > + || defined(__arm__) > > > > You should just put these on the same line: > > > > #if defined(arm) || defined(__arm__) > > Well, I would have, if surrounding #if's weren't split over several lines. So > please tell me which way you really prefer ;) Since the rest of the file is formatted with split lines, I think it's fine to leave it the way it is in the patch. Comment on attachment 16122 [details]
Patch v2
Ok. I'll r+ this but I'd prefer if whomever commits this would decrypt the ChangeLog message when doing so. FTBFS isn't a commonly-used acronym in this neck of the woods.
Comment on attachment 16122 [details]
Patch v2
r=me
Whoever lands this patch should change "FTBFS" to "build failure" in the ChangeLog.
Committed revision 25253. |