Closed
Bug 1203477
Opened 9 years ago
Closed 9 years ago
intl/hyphenation/ should be split in two, to separate third-party code from Mozilla code
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: sajitk)
Details
Attachments
(1 file, 5 obsolete files)
7.01 KB,
patch
|
Details | Diff | Splinter Review |
intl/hyphenation/README.mozilla explains that the following files are Mozilla-specific: hnjalloc.h hnjstdio.cpp nsHyphenationManager.cpp nsHyphenator.cpp moz.build Also, I'm pretty sure nsHyphenator.{h,cpp} should also be in that list. The other files in that directory come from the Hypen library, which is part of Hunspell. They should be in a separate directory.
Reporter | ||
Comment 1•9 years ago
|
||
BTW, intl/hyphenation/moz.build currently has ALLOW_COMPILER_WARNINGS=True in it because hyphen.c (one of the third-party files) has a compile warning. Once the split is complete, ALLOW_COMPILER_WARNINGS=True would be used for the third-party directory, but not used in the Mozilla code directory.
Reporter | ||
Comment 2•9 years ago
|
||
sajitk: this bug is similar to bug 1200065. Are you interested in doing it?
Flags: needinfo?(sajitk)
Sure, I will get started on it. Thanks.
Flags: needinfo?(sajitk)
Please find attached the patch file. I created "glue" and "hyphen" directories within /intl/hyphenation directory, and moved the hyphen library files into the "hyphen" and mozilla files to "glue". I think the README.mozilla file should be updated with this information, so if you agree with the approach, I will change the README.mozilla file as well.
Attachment #8659306 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8659306 [details] [diff] [review] 1203477.patch Review of attachment 8659306 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/hyphenation/hyphen/moz.build @@ +21,5 @@ > '-Wno-sign-compare', > '-Wno-type-limits', > ] > > if not CONFIG['GNU_CXX']: In intl/hyphenation/hyphen/moz.build you can replace all the stuff about warnings (the last 9 lines of the file) with a single line like this: > ALLOW_COMPILER_WARNINGS = True
Reporter | ||
Comment 7•9 years ago
|
||
Also, there's a file called tools/rewriting/ThirdPartyPaths.txt which lists third-party code in the tree. The "intl/hyphenation/" entry should be changed to "intl/hyphenation/hyphen/", and the hunspell entries should be updated for the changes you did in bug 1200065. (Sorry I didn't think of this until just now.)
Patch updated with last comments. Regarding the bug 1200065 and the ThirdPartyPaths.txt, the third-party Hunspell code source directory did not change, so the change may not be needed. I have updated this file for the hyphen code.
Attachment #8659306 -
Attachment is obsolete: true
Attachment #8659306 -
Flags: review?(mh+mozilla)
Attachment #8659357 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 9•9 years ago
|
||
> the third-party Hunspell code source directory did not change, so the change may not be > needed. It's not strictly necessary, but the current entries are like this: > extensions/spellcheck/hunspell/src/*.cxx > extensions/spellcheck/hunspell/src/*.hxx I think the "*.cxx" and "*.hxx" suffixes can now be removed because all the C++ code in that directory is now third-party. That will make these entries more like all the other ones in that file. (Given that bug 1200065's patch hasn't landed yet, that change could still be done in that bug.)
Comment 10•9 years ago
|
||
Comment on attachment 8659357 [details] [diff] [review] 1203477.patch Review of attachment 8659357 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/hyphenation/moz.build @@ +5,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +DIRS += [ > + 'glue', > + 'hyphen', It might be better to remove this file entirely, and to add hyphenation/glue and hyphenation/hyphen to the parent moz.build.
Attachment #8659357 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 11•9 years ago
|
||
Removed moz.build file from the hyphenation directory, as per review comment.
Attachment #8659357 -
Attachment is obsolete: true
Attachment #8659918 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8659918 [details] [diff] [review] 1203477.patch Review of attachment 8659918 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/hyphenation/README.mozilla @@ +20,5 @@ > > + glue/hnjalloc.h > + glue/hnjstdio.cpp > + glue/nsHyphenationManager.cpp > + glue/nsHyphenator.cpp I'd change this to just say "The source files in the glue/ directory are Mozilla-authored code...". It's more concise and won't become incorrect in the future if any files are added or removed.
Assignee | ||
Comment 13•9 years ago
|
||
Updated readme text.
Attachment #8659918 -
Attachment is obsolete: true
Attachment #8659918 -
Flags: review?(mh+mozilla)
Attachment #8660738 -
Flags: review?(mh+mozilla)
Comment 14•9 years ago
|
||
Comment on attachment 8660738 [details] [diff] [review] 1203477.patch Review of attachment 8660738 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/hyphenation/README.mozilla @@ +5,2 @@ > Hyphen library, part of the hunspell project. The various COPYING* and README* > files (except this README.mozilla) are likewise from the hunspell distribution Come to think of it, it would be best to move those files as well. @@ +15,5 @@ > Hyphen package from upstream, so the original README.* files may refer to > additional files that are not present in the Mozilla source tree. > > > +The source files in the /glue folder, as well as the build files You are adding trailing whitespaces in this file.
Attachment #8660738 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 15•9 years ago
|
||
The COPYING* and README* files are already moved to the new /hyphen folder. Please find attached the patch without the trailing whitespace in the Readme.mozilla. If OK, could you please also push to the try server?
Attachment #8660738 -
Attachment is obsolete: true
Attachment #8661245 -
Flags: review?(mh+mozilla)
Comment 16•9 years ago
|
||
Comment on attachment 8661245 [details] [diff] [review] 1203477.patch Review of attachment 8661245 [details] [diff] [review]: ----------------------------------------------------------------- ::: intl/hyphenation/README.mozilla @@ +7,1 @@ > of Hyphen: You can actually simplify all this by saying the hyphen/ directory comes from the hyphen library, part of the hunspell project, except moz.build. @@ +16,5 @@ > additional files that are not present in the Mozilla source tree. > > +The source files in the /glue directory, as well as the build files > +(Makefile.in and moz.build) are Mozilla-authored code, and the standard > +MPL 2.0 applies to these, as noted in the comments within the files. And you can remove this paragraph.
Attachment #8661245 -
Flags: review?(mh+mozilla) → review+
Here's a try run for it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c0ef4591b5a Thanks for the patch!
Assignee | ||
Comment 18•9 years ago
|
||
Sorry for my ignorance, but in the treeherder output, it looks like an OS2 build did not go through. From the output , it does not look like any issue with the code that was changed. Could you please let me know if I can set teh checkin-needed flag after making the last change as in Comment 16
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8661245 -
Attachment is obsolete: true
Comment 20•9 years ago
|
||
For good measure, I retriggered the job. Let's see how it goes the second time.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 21•9 years ago
|
||
Thank you. The last run seems to have worked. Requesting checkin-needed.
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77e7d5151129
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/77e7d5151129
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•