Closed
Bug 1064620
Opened 10 years ago
Closed 10 years ago
Considering changing preprocessing marker from # to //# in mobile/android Java code
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: nalexander, Assigned: ckitching)
Details
Attachments
(1 file, 2 obsolete files)
18.03 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
We would make the lives of IDE users simpler if we made our .java.in code syntactically valid. I don't see a way to make @...@ work, since /*@...@*/ would be missing a token, but there's little to lose from making our preprocessor marker //# instead of #. In this particular example, ckitching reports that Android Studio fails hard on #substitution on the first line of .java.in files.
Reporter | ||
Comment 1•10 years ago
|
||
ckitching: you could finish this off and see if it makes your life better. Comments on the ticket appreciated. I think I'd want feedback from mobile-firefox-dev before landing this. In particular, the platform crew (blassey, snorp, nchen) might not appreciate the context switch between Java and C++ code.
Attachment #8486113 -
Flags: feedback?(chriskitching)
Assignee | ||
Comment 2•10 years ago
|
||
Applied the pattern to all java.in files. This has made a real improvement: IDEA can finally resolve AppConstants references, among other things (some minor brokenness within the changes files where there are missing values, but that's not really a problem.
Attachment #8486120 -
Flags: review?(nalexander)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → chriskitching
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Attachment #8486113 -
Attachment is obsolete: true
Attachment #8486113 -
Flags: feedback?(chriskitching)
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8486120 [details] [diff] [review] Replace # with //# for IDE integration I have no doubt the patch is correct but will need to be updated for landing. This is blocked on consensus, not on technical correctness. We'll reflag when we have consensus.
Attachment #8486120 -
Flags: review?(nalexander)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8486120 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
(Now with more qref: this one actually builds) *whistles innocently*
Assignee | ||
Comment 6•10 years ago
|
||
*pokes with a stick* How do we perform the required politics to make this land? ... Does anyone have a notion of what the arguments against doing this are? It feels like we just stalled right before doing a really neat thing for an intangible reason: "it might annoy someone".
(In reply to Chris Kitching [:ckitching] from comment #6) > How do we perform the required politics to make this land? I'd say mail the list and if no one objects after a few days, land it. If they object afterwards, they can comment in the bug and it can later be backed out. > ... Does anyone have a notion of what the arguments against doing this are? Perhaps: * Less visible because syntax highlighters (sometimes) recognize `#`, but will just mark `//#` as a comment. * Less visible if the comments are indented, as comments usually are, instead of at column 1 (I would say don't indent) * Scripts may pick up pre-processing with a `^#` regex so you might break some things (seems to be an easy fix though) - I doubt anything important does this though
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8486897 [details] [diff] [review] Replace # with //# for IDE integration Review of attachment 8486897 [details] [diff] [review]: ----------------------------------------------------------------- Since nobody appears to care, and it makes Nick's Gradle-based IDEA support very slightly less ridiculously awesome than it already is, I can haz review to land this?
Attachment #8486897 -
Flags: review?(rnewman)
Comment 9•10 years ago
|
||
Comment on attachment 8486897 [details] [diff] [review] Replace # with //# for IDE integration Review of attachment 8486897 [details] [diff] [review]: ----------------------------------------------------------------- Rubberstamp. Ping Nick if you want a real build opinion!
Attachment #8486897 -
Flags: review?(rnewman) → review+
Comment 10•10 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/fx-team/rev/88fb275bfd3e https://treeherder.mozilla.org/ui/logviewer.html#?job_id=874275&repo=fx-team
Assignee | ||
Comment 11•10 years ago
|
||
Learning the hard way that ^Cing a push is a bad plan... Looks like someone sneaked an extra few preprocessor directives into AppConstants since the patch was last checked. It didn't conflict with the other chunks but caused asplosions. Pushing without building was a stupid thing to try to do :/ . I'll land the fixed patch when the tree opens. Sorry for making merge weekend more "exciting"!
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b557f2cb4a04
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b557f2cb4a04
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 35 → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•