Closed Bug 956659 Opened 10 years ago Closed 10 years ago

Remove the keyboard/js/imes/ files we are not enabling

Categories

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Keyboard
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: timdream, Assigned: timdream)

Details

(Whiteboard: tarako)

Attachments

(1 file)

mozilla-b2g:master PR#15035
46 bytes, text/x-github-pull-request
yurenju
: review+
djf
: review+
Details | Review 
Zhuyin and Pinyin database are not being removed by the keyboard build script, resulting ~6.4MB (before compression) of extra data in the ROM.

Need a quick patch because of bug 944457
Looks doable from build/keyboard-config.js after |git mv -r|.
Assignee: nobody → timdream
Is this modification right?

-GAIA_KEYBOARD_LAYOUTS?=en,pt-BR,es,de,fr,pl
+GAIA_KEYBOARD_LAYOUTS?=en#,pt-BR,es,de,fr,pl
(In reply to James Zhang from comment #2)
> Is this modification right?
> 
> -GAIA_KEYBOARD_LAYOUTS?=en,pt-BR,es,de,fr,pl
> +GAIA_KEYBOARD_LAYOUTS?=en#,pt-BR,es,de,fr,pl

Yes, but this bug is about removing more extra files following this variable. No action from your side required.
Attachment #8356038 - Flags: review?(yurenju.mozilla)
Attachment #8356038 - Flags: review?(dflanagan)
Comment on attachment 8356038 [details] [review]
mozilla-b2g:master PR#15035

looks good to me, thank you!
Attachment #8356038 - Flags: review?(yurenju.mozilla) → review+
blocking-b2g: fugu? → 1.3?
Whiteboard: tarako
Set this bug to 1.3+ based on comment 0.
blocking-b2g: 1.3? → 1.3+
Comment on attachment 8356038 [details] [review]
mozilla-b2g:master PR#15035

This is great, Tim. Thanks for doing it!

Please check your negation syntax in the .gitignore file. I have not tested it myself, but it doesn't look right to me.

(As a more general matter, I've learned from the engineers refactoring the camera app that Yuren's build system now allows apps to manage their own build directory. Had I known that when I did the configurable dictionary patch, I think I would have tried to use that feature so I didn't have to create the gaia/keyboards/ directory. I wonder if we should eventually move gaia/keyboards/* back into apps/keyboard/. Obviously not for this bug, however.)
Attachment #8356038 - Flags: review?(dflanagan) → review+
I have made some tests to ensure the syntax is correct: https://github.com/mozilla-b2g/gaia/pull/15035#r8746495

master: https://github.com/mozilla-b2g/gaia/commit/6c2dac3444e0d68eb81677434d9c166692d8d247
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to David Flanagan [:djf] from comment #7)
> (As a more general matter, I've learned from the engineers refactoring the
> camera app that Yuren's build system now allows apps to manage their own
> build directory. Had I known that when I did the configurable dictionary
> patch, I think I would have tried to use that feature so I didn't have to
> create the gaia/keyboards/ directory. I wonder if we should eventually move
> gaia/keyboards/* back into apps/keyboard/. Obviously not for this bug,
> however.)

If we are going to replace the current keyboard it doesn't make sense to work on it's build script further. Please leverage this feature in the new keyboard app.
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 6c2dac3444e0d68eb81677434d9c166692d8d247
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(timdream)
v1.3: c3b12321f4a6a3a63d686624f084ae0930c82c33
status-b2g-v1.3: --- → fixed
Flags: needinfo?(timdream)
Hey Tim, this broke unit tests on v1.3, see https://travis-ci.org/mozilla-b2g/gaia/jobs/16705568

Therefore I'm backing it out:
revert on v1.3: 67b35df4ab2ee428b32ca041fb95a9383642a253
status-b2g-v1.3: fixed → affected
Flags: needinfo?(timdream)
(In reply to Julien Wajsberg [:julienw] from comment #12)
> Hey Tim, this broke unit tests on v1.3, see
> https://travis-ci.org/mozilla-b2g/gaia/jobs/16705568
> 
> Therefore I'm backing it out:
> revert on v1.3: 67b35df4ab2ee428b32ca041fb95a9383642a253

Ouch.

These unit tests failed because the patch removes the files it tests. Proper solution I have in mind would be only include these tests if the JSPinyin is enabled, but that's something to complicated to do in this bug.

I will be disabling JSPinyin tests in v1.3 and file another bug for the proper fix.
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> These unit tests failed because the patch removes the files it tests. Proper
> solution I have in mind would be only include these tests if the JSPinyin is
> enabled, but that's something to complicated to do in this bug.
> 
> I will be disabling JSPinyin tests in v1.3 and file another bug for the
> proper fix.

Follow-up on bug 959057
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> (In reply to Julien Wajsberg [:julienw] from comment #12)
> > Hey Tim, this broke unit tests on v1.3, see
> > https://travis-ci.org/mozilla-b2g/gaia/jobs/16705568
> > 
> > Therefore I'm backing it out:
> > revert on v1.3: 67b35df4ab2ee428b32ca041fb95a9383642a253
> 
> Ouch.
> 
> These unit tests failed because the patch removes the files it tests. Proper
> solution I have in mind would be only include these tests if the JSPinyin is
> enabled, but that's something to complicated to do in this bug.
> 
> I will be disabling JSPinyin tests in v1.3 and file another bug for the
> proper fix.

I don't understand why the patch does not removes the tests as well, if it removes the files it tests?

Is the feature working in 1.3 on the device?
(In reply to Julien Wajsberg [:julienw] from comment #17)
> 
> I don't understand why the patch does not removes the tests as well, if it
> removes the files it tests?

Yeah it probably should have. But with what we have right now I think we should fix it in a follow-up bug rather than back it out.

> Is the feature working in 1.3 on the device?

It works as long as you don't hit bug 946068, for now.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #18)
> (In reply to Julien Wajsberg [:julienw] from comment #17)
> > 
> > I don't understand why the patch does not removes the tests as well, if it
> > removes the files it tests?
> 
> Yeah it probably should have. But with what we have right now I think we
> should fix it in a follow-up bug rather than back it out.

Wait, we actually cannot do that. Build script can only mutate the package not the source tree, and the unit test runner will run tests in the source tree. We don't ship tests in packages (and we shouldn't).
I think I really don't get what's going on so I'll trust you on this ;) as long as it's green it's fine!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: