extensions/spellcheck/hunspell/src should be split into two directories, to separate third-party code from Mozilla code. This will make things nicer for the build system, particularly in relation to compiler warnings. Specifically, the third-party code should be built with ALLOW_COMPILER_WARNINGS, while the Mozilla code should not. This will allow the following lines in the moz.build file to be removed. > # Suppress warnings in third-party code. > if CONFIG['CLANG_CXX']: > CXXFLAGS += ['-Wno-unused-private-field'] It's also possible that MOZ_HUNSPELL_CFLAGS could be involved in this change, but I'm not certain about that.

To fix this bug, take a look at <https://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/moz.build#15>. Everything else that this moz.build references is mozilla specific code, and they should be moved to a new directory called extensions/spellcheck/hunspell/glue. It mostly involves moving the code, adjusting the moz.build files, and getting the code to compile and link properly.

Hi, Please find attached a patch for this bug. I have compiled successfully, and run the tests that I could find. Please let me know next steps

Comment on attachment 8656130 [details] [diff] [review] 1200065.patch Review of attachment 8656130 [details] [diff] [review]: ----------------------------------------------------------------- Hi, sajitk! Thank you and congratulations for uploading your first patch :) I'm not an expert on the build system but I do know enough to identify a few issues with your patch, which I've described below. Please fix them. Once you've done that, please upload a new patch, and then you'll need to request review using the "review" flag. Set it to "?" and put ":glandium" as the reviewer. (He's somebody who *is* an expert on the build system.) And don't feel bad about having to fix some issues; that's common for patches written even by experienced Mozilla developers and pretty much universal for newcomers :) I'm giving the "feedback+" flag which means the patch is heading in the right direction but is not quite there yet. ---- Just to check: did you use |hg mv| to move the files from src/ to glue/? If you did, that's great, because it preserves file history. Relatedly, you've moved all the Mozilla-specific files into the new glue directory, which is good. But it looks like you also moved all the Hunspell code (with .hxx and .cxx extensions) too. I.e. the patch has lots of lines like this: > rename from extensions/spellcheck/hunspell/src/atypes.hxx > rename to extensions/spellcheck/hunspell/glue/atypes.hxx These files should stay in the src directory. Due to this, I'm actually surprised that the code compiled. I could be wrong, but I would have thought this would cause compiler errors. ::: extensions/spellcheck/hunspell/glue/moz.build @@ +2,5 @@ > +# vim: set filetype=python: > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > +SOURCES += [ Add a blank line between the comment and the SOURCES line. @@ +8,5 @@ > + 'mozHunspellDirProvider.cpp', > + 'RemoteSpellCheckEngineChild.cpp', > + 'RemoteSpellCheckEngineParent.cpp', > +] > +CXXFLAGS += CONFIG['MOZ_HUNSPELL_CFLAGS'] I think MOZ_HUNSPELL_CFLAGS should only be applied to the Hunspell code, not the Mozilla glue code, so this line can be removed. @@ +22,5 @@ > + > +# XXX: This directory is a mix of Mozilla code and third-party code. We should > +# put the Mozilla code in a separate directory and disallow compiler warnings > +# there (bug 1200065). Until then, allow warnings for all of the code. > +ALLOW_COMPILER_WARNINGS = False This comment is now out of date and should be removed. Also, the default value for ALLOW_COMPILER_WARNINGS is False so you can remove that line too. ::: extensions/spellcheck/hunspell/src/moz.build @@ -14,5 @@ > -if not CONFIG['MOZ_NATIVE_HUNSPELL']: > - SOURCES += [ > - 'affentry.cxx', > - 'affixmgr.cxx', > - 'csutil.cxx', I think you want to keep this if/else unchanged. The MOZ_NATIVE_HUNSPELL flag must be checked for the --enable-system-hunspell configure flag to be respected. (See configure.in for the definition of that configure flag.) @@ +30,1 @@ > '/extensions/spellcheck/src', I'm not certain, but it's likely that not all of these entries need to be here any more, particularly /dom/base and /editor/libeditor. @@ +35,1 @@ > ALLOW_COMPILER_WARNINGS = True Remove this comment. There's no need to describe how the code used to look. @@ +35,3 @@ > ALLOW_COMPILER_WARNINGS = True > > include('/ipc/chromium/chromium-config.mozbuild') I think you can remove this line, now that it's in the new moz.build file. The chromium headers are only used by the glue code, not by Hunspell's code.

(In reply to Nicholas Nethercote [:njn] from comment #3) > @@ +8,5 @@ > > + 'mozHunspellDirProvider.cpp', > > + 'RemoteSpellCheckEngineChild.cpp', > > + 'RemoteSpellCheckEngineParent.cpp', > > +] > > +CXXFLAGS += CONFIG['MOZ_HUNSPELL_CFLAGS'] > > I think MOZ_HUNSPELL_CFLAGS should only be applied to the Hunspell code, not > the Mozilla glue code, so this line can be removed. Actually, MOZ_HUNSPELL_CFLAGS is for when building against the system hunspell, so it is meant for the Mozilla glue code, not the Hunspell code :) It's not conditionally added because it's empty when building with the in-tree hunspell.

> Actually, MOZ_HUNSPELL_CFLAGS is for when building against the system > hunspell, so it is meant for the Mozilla glue code, not the Hunspell code :) Thank you for the correction. (Like I said, glandium is the real expert on the build system :)

Thank you both, the feedback is much appreciated. I have attached an updated patch with the changes you have suggested. I think I have got all of them, I did have one question about the files hunspell_alloc_hooks.h and hunspell_fopen_hooks.h. These seem to be Mozilla code needed to add features to hunspell functions, so I moved these files to the glue folder. However, they are included from the hunspell code (hunspell.hxx) so I have LOCAL_INCLUDE in the src folder #include-ing these files from glue. The glue folder code also #include-s the src header files. Please let me know if the files should stay in the src folder, thus removing the need for the circular includes.

Comment on attachment 8656815 [details] [diff] [review] 1200065.patch Review of attachment 8656815 [details] [diff] [review]: ----------------------------------------------------------------- This one looks better. I only saw one small issue, but glandium may find more. Thank you. ::: extensions/spellcheck/hunspell/glue/moz.build @@ +18,5 @@ > +LOCAL_INCLUDES += [ > + '../src', > + '/dom/base', > + '/editor/libeditor', > + '/extensions/spellcheck/src', Looks like you have a tab character in front of the first entry. Mozilla code always avoids tabs in favour of spaces.

Comment on attachment 8656815 [details] [diff] [review] 1200065.patch Review of attachment 8656815 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to sajitk from comment #6) > Created attachment 8656815 [details] [diff] [review] > 1200065.patch > > Thank you both, the feedback is much appreciated. I have attached an updated > patch with the changes you have suggested. I think I have got all of them, > > I did have one question about the files hunspell_alloc_hooks.h and > hunspell_fopen_hooks.h. These seem to be Mozilla code needed to add features > to hunspell functions, so I moved these files to the glue folder. However, > they are included from the hunspell code (hunspell.hxx) so I have > LOCAL_INCLUDE in the src folder #include-ing these files from glue. The glue > folder code also #include-s the src header files. Please let me know if the > files should stay in the src folder, thus removing the need for the circular > includes. You did it exactly right :) There are only a few nits I would like to see addressed before giving a definite r+, see below. Sorry, I should have looked at the patch instead of replying to Nick's previous comment, that would have avoided one round-trip. > rename from extensions/spellcheck/hunspell/src/README.mozilla > rename to extensions/spellcheck/hunspell/glue/README.mozilla README.mozilla contains information about the hunspell library version and additional patches. It would be better to keep it where it is. Or, it could be moved one level up under extensions/spellcheck/hunspell along with the patches/ subdirectory. I don't have a strong opinion about the latter. ::: extensions/spellcheck/hunspell/glue/moz.build @@ +1,1 @@ > +# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*- Please make this file a copy of extensions/spellcheck/hunspell/src/moz.build so that the patch looks like it's removing lines from there. This way, history of the lines copied from there will still be attributed to when they were actually added in that file, rather than your change. ::: extensions/spellcheck/hunspell/src/moz.build @@ +7,1 @@ > if not CONFIG['MOZ_NATIVE_HUNSPELL']: This condition could be moved to the parent moz.build, only adding the 'src' directory to DIRS under this condition. @@ +22,4 @@ > # too if you need to change this variable. > DEFINES['HUNSPELL_STATIC'] = True > else: > CXXFLAGS += CONFIG['MOZ_HUNSPELL_CFLAGS'] This line is not needed anymore ::: extensions/spellcheck/src/moz.build @@ +23,1 @@ > '../hunspell/src', A cursory look seems to indicate we don't need headers from the hunspell source in this directory.

I apologize for yet another round trip, I have attached a new patch, but I don't think I have it correct yet. Based on your last feedback: >>. > if not CONFIG['MOZ_NATIVE_HUNSPELL']: >>This condition could be moved to the parent moz.build, only adding the 'src' >>directory to DIRS under this condition. I interpreted this to be if not CONFIG['MOZ_NATIVE_HUNSPELL']: DIRS += ['glue', 'src'] else: DIRS += ['glue'] Anything other than that was causing link errors. Please let me know if this is not what you meant. I suspect that the same link error will be caused if the else condition is exercised, so I wasnt sure whether what I have done is correct. Please advise. 2. >> > '../hunspell/src', >>A cursory look seems to indicate we don't need headers from the hunspell >>source in this directory. Without this entry, the compiler is not able to find the hunspell.hxx header file included from the glue directory. So I have left it in.

Comment on attachment 8657398 [details] [diff] [review] 1200065.patch Review of attachment 8657398 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. We're almost there. A couple trivial things to fix-up and we're all set to land. ::: extensions/spellcheck/hunspell/moz.build @@ +4,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +if not CONFIG['MOZ_NATIVE_HUNSPELL']: > + DIRS += ['glue', 'src'] This would be simpler as: DIRS += ['glue'] if not CONFIG['MOZ_NATIVE_HUNSPELL']: DIRS += ['src'] ::: extensions/spellcheck/hunspell/src/moz.build @@ +7,1 @@ > if not CONFIG['MOZ_NATIVE_HUNSPELL']: Now that src is only added to DIRS when not CONFIG['MOZ_NATIVE_HUNSPELL'], this test is of no use. Please remove it.

Thanks again, appreciate your patience. Please find attached the latest version of the patch. If this is ok, could you please point me to the next steps to close this out?

Comment on attachment 8658277 [details] [diff] [review] 1200065.patch Review of attachment 8658277 [details] [diff] [review]: ----------------------------------------------------------------- The next step here would be to address the final comment below, add a commit message to your patch (see https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions ) and then follow the instructions on https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree Here is a push to the try server, already: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff950ecc0bba ::: extensions/spellcheck/hunspell/moz.build @@ +10,4 @@ > > if CONFIG['ENABLE_TESTS']: > XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini'] > + You have extra whitespaces added at the end of this file.

Please find attached the final patch. Could you please set the 'checkin-needed' keyword for the patch as I dont have permissions? Thanks and regards,

You're still missing a commit message in your patch (see https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions )

Please find attached

Changed the hunspell directory paths in the ThirdPartyPaths.txt file

Congratulations on landing your first patch in the Mozilla codebase, sajitk! First bugs often involve a lot of back and forth with the reviewer, as was the case with this one, but it gets easier with time and experience :) If you are interested in working on more bugs, you might find this website useful: http://www.joshmatthews.net/bugsahoy/

I had to back this out of aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/18d549918ec0 so I could easily back out bug 697981. After I backed that out, I relanded this in https://hg.mozilla.org/releases/mozilla-aurora/rev/04b17a8de4ed