Closed Bug 864774 Opened 12 years ago Closed 11 years ago

Move CPPSRCS to moz.build

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: mshal, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

Part 1: Support CPP_SOURCES in moz.build
2.43 KB, patch
gps
: review+
Details | Diff | Splinter Review
Part 2: Move CPPSRCS to moz.build as CPP_SOURCES
817.31 KB, patch
gps
: review+
Details | Diff | Splinter Review
comm-central migration
74.05 KB, patch
standard8
: review+
Details | Diff | Splinter Review
part 3 - Move some more CPPSRCS to moz.build
5.19 KB, patch
mshal
: review+
Details | Diff | Splinter Review
CPPSRCS should be an easy one to move to moz.build. I believe this can just be a list in moz.build using a VariablePassthru.
Attachment #743649 - Flags: review?(gps)
Some CPP_SOURCES are still set referring to other make variables (eg: $(...LCPPSRCS)) which are defined in objs.mk. This will be deferred until bug 865673.
Attachment #743653 - Flags: review?(joey)
comm-central version hasn't been tested on try, but it builds locally.
Attachment #743655 - Flags: review?(joey)
Comment on attachment 743649 [details] [diff] [review] Part 1: Support CPP_SOURCES in moz.build Review of attachment 743649 [details] [diff] [review]: ----------------------------------------------------------------- When are you planning to port CSRCS and CMSRCS? Hopefully soon because IMO they are related and it's confusing to have different locations for each.
Attachment #743649 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #4) > When are you planning to port CSRCS and CMSRCS? Hopefully soon because IMO > they are related and it's confusing to have different locations for each. I'll be tackling those after this one goes through. I agree it's confusing, but it's a lot to move all at once.
Comment on attachment 743653 [details] [diff] [review] Part 2: Move CPPSRCS to moz.build as CPP_SOURCES build/stlport/moz.build: CPP_SOURCES += [ '$(notdir $(wildcard $(STLPORT_SOURCES)/src/*.cpp))', ] Have not actually tried this to see if the makefile function calls might succeed during expansion but the syntax is odd. Probably will not work ayway until STLPORT_SOURCES is defined.
Using make constructs in variable values kind of defeats the further goal of moz.build...
(In reply to Mike Hommey [:glandium] from comment #7) > Using make constructs in variable values kind of defeats the further goal of > moz.build... Indeed. It will still work as long as the variables are using variable passthru. I'm willing to let this slide in areas. But, it should be used sparingly and there should be a clear plan to remove the hackiness. I think the immediate goal of moving as much as possible to moz.build trumps temporary hackiness like this.
Comment on attachment 743653 [details] [diff] [review] Part 2: Move CPPSRCS to moz.build as CPP_SOURCES content/media/webaudio/blink/ ============================= Biquad.cpp exists in the patch but not in mozilla-central. if not an intentional addition maybe a patch was backed out ? content/svg/content/src/Makefile.in =================================== [m-c] > nsDOMSVGZoomEvent.cpp [864774] < SVGZoomEvent.cpp dom/bindings/Makefile.in ======================== CPP_SOURCES += [ '$(linked_binding_cpp_files)', 'RegisterBindings.cpp', ... ] embedding/somponents/commandhandler/src/Makefile.in CPPSRCS = still listed in Makefile.in gfx/angle/src/libGLESv2/Makefile.in o one of -- stray comments removed, 'Target:' changed from 'translator_common' to 'preprocessor'. # Target: 'translator_common' # Requires: 'preprocessor' LOCAL_INCLUDES = \
Comment on attachment 743653 [details] [diff] [review] Part 2: Move CPPSRCS to moz.build as CPP_SOURCES PATH: ipc/ipdl/moz.build MODULE = 'ipdlgen' CPP_SOURCES += [ '$(PROTOCOLS:%.ipdl=%.cpp)', '$(PROTOCOLS:%.ipdl=%Child.cpp)', '$(PROTOCOLS:%.ipdl=%Parent.cpp)', '$(PROTOCOLS:%.ipdlh=%.cpp)', ]
(In reply to Joey Armstrong [:joey] from comment #10) > Comment on attachment 743653 [details] [diff] [review] > Part 2: Move CPPSRCS to moz.build as CPP_SOURCES > > PATH: ipc/ipdl/moz.build > > MODULE = 'ipdlgen' > > CPP_SOURCES += [ > '$(PROTOCOLS:%.ipdl=%.cpp)', > '$(PROTOCOLS:%.ipdl=%Child.cpp)', > '$(PROTOCOLS:%.ipdl=%Parent.cpp)', > '$(PROTOCOLS:%.ipdlh=%.cpp)', > ] Perhaps IPDLs should be excluded from this conversion. They probably merit their own consideration.
(In reply to comment #9) > Comment on attachment 743653 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=743653 > Part 2: Move CPPSRCS to moz.build as CPP_SOURCES > > content/media/webaudio/blink/ > ============================= > Biquad.cpp exists in the patch but not in mozilla-central. > if not an intentional addition maybe a patch was backed out ? It does exist in mozilla-central: http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/blink/Biquad.cpp?force=1. Perhaps your tree is about a week or so old?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12) > (In reply to comment #9) > > Comment on attachment 743653 [details] [diff] [review] > > --> https://bugzilla.mozilla.org/attachment.cgi?id=743653 > > Part 2: Move CPPSRCS to moz.build as CPP_SOURCES > > > > content/media/webaudio/blink/ > > ============================= > > Biquad.cpp exists in the patch but not in mozilla-central. > > if not an intentional addition maybe a patch was backed out ? > > It does exist in mozilla-central: > http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/blink/ > Biquad.cpp?force=1. Perhaps your tree is about a week or so old? Hmmm my comparison sandbox was updated, I'll checkout a new one just in case.
(In reply to Gregory Szorc [:gps] from comment #8) > (In reply to Mike Hommey [:glandium] from comment #7) > > Using make constructs in variable values kind of defeats the further goal of > > moz.build... > > Indeed. It will still work as long as the variables are using variable > passthru. > > I'm willing to let this slide in areas. But, it should be used sparingly and > there should be a clear plan to remove the hackiness. > > I think the immediate goal of moving as much as possible to moz.build trumps > temporary hackiness like this. I'll just continue to flag any unexpanded references like this. If they were intended great, if not will have a chance for cleanup.
If the conversion patch does not land right away maybe open a separate bug for this cleanup netwerk/base/src/Makefile.in -- bugfix ====================================== CPPSRCS = \ + ArrayBufferInputStream.cpp \
toolkit/crashreporter/breakpad-windows-libxul ============================================= CPP_SOURCES += [ < '$(objs_common)', < '$(objs_crash_generation)', < '$(objs_handler)', < '$(objs_sender)', < 'http_upload.cc', < ] < tools/trace-malloc/moz.build ============================ CPP_SOURCES += [ < '$(EXTRACPP_SOURCES)', < '$(SIMPLECPP_SOURCES)', < ] < < widget/xremoteclient/moz.build ============================== CPP_SOURCES += [ < '$(filter-out $(LIBCPPSRCS),$(PROGCPPSRCS)) $(LIBCPPSRCS) < ] < < xpcom/glue/standalone/ =========================================================================== CPP_SOURCES += [ < '$(XPCOM_GLUE_SRC_LCPPSRCS)', < 'nsStringAPI.cpp', < 'nsXPCOMGlue.cpp', < ] < New var added in m-c $(LINKSRC) xpcom/reflect/xptcall/src/md/test/Makefile.in ============================================= 'invoke_test.cpp' is commented and was not transferred. Source could be added or a bug opened to enable or cleanup > CPPSRCS = stub_test.cpp #invoke_test.cpp CPP_SOURCES += [ < 'stub_test.cpp', < ] <
accessible/src/base/moz.build ============================= ifneq ($(A11Y_LOG),0) CPPSRCS += \ Logging.cpp \ $(NULL) endif if CONFIG['A11Y_LOG'] != '0': CPP_SOURCES += [ 'Logging.cpp', ] Dropping "!= '0'" would harden the conditional so non-numeric/unset values cannot cause a problem. content/svg/content/src/Makefile.in - not converted
embedding/components/commandhandler/src/Makefile.in gfx/layers/Makefile.in - CPPSRCS still referenced in the makefile. intl/unicharutil/util/internal/moz.build ======================================== CPP_SOURCES += [ '$(INTL_UNICHARUTIL_UTIL_LCPPSRCS)', ] ipc/ipdl/test/cxx/moz.build =========================== CPP_SOURCES += [ '$(IPDLTESTSRCS)', 'IPDLUnitTestProcessChild.cpp', 'IPDLUnitTestSubprocess.cpp', 'IPDLUnitTests.cpp', ]
Comment on attachment 743653 [details] [diff] [review] Part 2: Move CPPSRCS to moz.build as CPP_SOURCES 847 files - some cleanups and cosmetic edits but the bulk of edits look good.
Attachment #743653 - Flags: review?(joey) → review+
(In reply to Joey Armstrong [:joey] from comment #9) > Comment on attachment 743653 [details] [diff] [review] > Part 2: Move CPPSRCS to moz.build as CPP_SOURCES > > content/media/webaudio/blink/ > ============================= > Biquad.cpp exists in the patch but not in mozilla-central. > if not an intentional addition maybe a patch was backed out ? Ehsan already covered this one - it's a new file and should be listed in moz.build > > > content/svg/content/src/Makefile.in > =================================== > [m-c] > nsDOMSVGZoomEvent.cpp > [864774] < SVGZoomEvent.cpp I've rebased to include this change. > > > dom/bindings/Makefile.in > ======================== > CPP_SOURCES += [ > '$(linked_binding_cpp_files)', > 'RegisterBindings.cpp', > ... > ] > I filed bug 868539 for cleaning up this Makefile syntax in moz.build files. It's a lot to do all at once and I think is out of the scope of this particular bug. > > embedding/somponents/commandhandler/src/Makefile.in > CPPSRCS = still listed in Makefile.in It looks like CPPSRCS is removed in this directory according to the patch I posted - maybe you found it in another directory? > > > gfx/angle/src/libGLESv2/Makefile.in > o one of -- stray comments removed, 'Target:' changed from > 'translator_common' to 'preprocessor'. > > # Target: 'translator_common' > # Requires: 'preprocessor' > LOCAL_INCLUDES = \ Good catch - these should be updated. It seems the auto-mozbuild converter is deleting comments after CPPSRCS rather than the ones before it.
(In reply to Joey Armstrong [:joey] from comment #16) > toolkit/crashreporter/breakpad-windows-libxul > ============================================= > CPP_SOURCES += [ < > '$(objs_common)', < > '$(objs_crash_generation)', < > '$(objs_handler)', < > '$(objs_sender)', < > 'http_upload.cc', < > ] < > > > tools/trace-malloc/moz.build > ============================ > CPP_SOURCES += [ < > '$(EXTRACPP_SOURCES)', < > '$(SIMPLECPP_SOURCES)', < > ] < > < > > widget/xremoteclient/moz.build > ============================== > CPP_SOURCES += [ < > '$(filter-out $(LIBCPPSRCS),$(PROGCPPSRCS)) $(LIBCPPSRCS) < > ] < > < > These will be deferred until bug 868539. > xpcom/glue/standalone/ > =========================================================================== > CPP_SOURCES += [ < > '$(XPCOM_GLUE_SRC_LCPPSRCS)', < > 'nsStringAPI.cpp', < > 'nsXPCOMGlue.cpp', < > ] < > > New var added in m-c $(LINKSRC) > The definition of LINKSRC was actually removed in a recent change, so it evaluates to an empty string even though it was still referenced in CPPSRCS. It isn't defined anywhere else in the tree, so is safe to remove from the moz.build file. > > xpcom/reflect/xptcall/src/md/test/Makefile.in > ============================================= > > 'invoke_test.cpp' is commented and was not transferred. Source could be > added or a bug opened to enable or cleanup > > > CPPSRCS = stub_test.cpp #invoke_test.cpp > > CPP_SOURCES += [ < > 'stub_test.cpp', < > ] < It looks like invoke_test.cpp was commented out in 2003. I think it's probably safe to just remove it from the moz.build file.
(In reply to Joey Armstrong [:joey] from comment #17) > accessible/src/base/moz.build > ============================= > > ifneq ($(A11Y_LOG),0) > CPPSRCS += \ > Logging.cpp \ > $(NULL) > endif > > if CONFIG['A11Y_LOG'] != '0': > CPP_SOURCES += [ > 'Logging.cpp', > ] > > Dropping "!= '0'" would harden the conditional so non-numeric/unset values > cannot cause a problem. Ok, done. > > > content/svg/content/src/Makefile.in > - not converted It looks like CPPSRCS is removed in the patch... I've rebased this directory though to include the SVGZoomEvent.cpp change.
(In reply to Michael Shal [:mshal] from comment #22) > (In reply to Joey Armstrong [:joey] from comment #17) > > accessible/src/base/moz.build > > ============================= > > > > ifneq ($(A11Y_LOG),0) > > CPPSRCS += \ > > Logging.cpp \ > > $(NULL) > > endif > > > > if CONFIG['A11Y_LOG'] != '0': > > CPP_SOURCES += [ > > 'Logging.cpp', > > ] > > > > Dropping "!= '0'" would harden the conditional so non-numeric/unset values > > cannot cause a problem. > > Ok, done. Actually, it seems A11Y_LOG is not a config variable, but exported to the environment from accessible/Makefile.in, so we can't use it here. Right now this would be in the only moz.build file using A11Y_LOG, but we will need it in other moz.build files once DEFINES are moved over. gps: Do you have a suggestion for how to share a variable definition like this? Can we define A11Y_LOG in some moz build type file in accessible/ that is automatically included for all subdirs of accessible?
Flags: needinfo?(gps)
I don't think we should be exporting variables like that. I say move it to configure or config.mk, renaming as necessary.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #24) > I don't think we should be exporting variables like that. I say move it to > configure or config.mk, renaming as necessary. Are you sure? This seems more like a local variable than a configure variable. Its definition in accessible/src/Makefile.in is: A11Y_LOG = 0 ifdef MOZ_DEBUG A11Y_LOG = 1 endif ifeq (,$(filter aurora beta release esr,$(MOZ_UPDATE_CHANNEL))) A11Y_LOG = 1 endif It is just exported to the environment as a way to share it with sub-makes. In moz.build land, I would like to be able to create a accessible/src/shared.mozbuild (or something) that gets included by accessible/src/moz.build, accessible/src/base/moz.build, etc. So we could do something like: accessible/src/shared.mozbuild: a11y_log = 0 if CONFIG['MOZ_DEBUG'] or CONFIG['MOZ_UPDATE_CHANNEL'] in ('aurora', 'beta', 'release', esr'): a11y_log = 1 accessible/src/base/moz.build: if a11y_log: DEFINES += blah Granted we can do this now with include, but having a bunch of 'include "../../shared.mozbuild" lines is kinda ugly.
Comment on attachment 743655 [details] [diff] [review] Move CPPSRCS to moz.build as CPP_SOURCES (comm-central) calendar/sunbird/app/Makefile.in ================================ CPPSRCS += $(STATIC_CPPSRCS) mailnews/base/src/moz.build =========================== ** Some assignments in this file have a space suffixed on the filename. if CONFIG['OS_ARCH'] == 'WINNT': CPP_SOURCES += [ 'nsMessengerWinIntegration.cpp ' ] 'nsMessengerWinIntegration.cpp ' 'nsMessengerOS2Integration.cpp ' 'nsMessengerUnixIntegration.cpp ' [...]
Comment on attachment 743655 [details] [diff] [review] Move CPPSRCS to moz.build as CPP_SOURCES (comm-central) 102 files, suffixed space in mailnews/base/src/moz.build was the only problem.
Attachment #743655 - Flags: review?(joey) → review+
(In reply to Joey Armstrong [:joey] from comment #26) > Comment on attachment 743655 [details] [diff] [review] > Move CPPSRCS to moz.build as CPP_SOURCES (comm-central) > > calendar/sunbird/app/Makefile.in > ================================ > CPPSRCS += $(STATIC_CPPSRCS) This will be handled in bug 866903, since that Makefile (or at least that part of the Makefile) isn't actually used. > > > mailnews/base/src/moz.build > =========================== > ** Some assignments in this file have a space suffixed on the filename. > > if CONFIG['OS_ARCH'] == 'WINNT': > CPP_SOURCES += [ > 'nsMessengerWinIntegration.cpp ' > ] > > 'nsMessengerWinIntegration.cpp ' > 'nsMessengerOS2Integration.cpp ' > 'nsMessengerUnixIntegration.cpp ' > [...] Not sure how that got in there - good catch!
gps: Please review accessible/src/shared.mozbuild and accessible/src/base/moz.build for use of a11y_log variable. joey has already reviewed the rest of the patch, and his feedback is included in this version.
Attachment #743653 - Attachment is obsolete: true
Attachment #746384 - Flags: review?(gps)
r=joey carried forward
Attachment #743655 - Attachment is obsolete: true
Comment on attachment 746384 [details] [diff] [review] Part 2: Move CPPSRCS to moz.build as CPP_SOURCES Review of attachment 746384 [details] [diff] [review]: ----------------------------------------------------------------- I still think general refactoring of a11y_log is in order since it quacks like a configure-time variable. But, this shouldn't block this work. 2 steps forward 1 backward is still progress.
Attachment #746384 - Flags: review?(gps) → review+
Whiteboard: [leave open]
Um, might have been nice to hold off on this until after the integration.. Anyone with a patch to land will now need to integrate with this change... Minor problem but one that greatly increases the chance of someone busting the tree inadvertently.
(In reply to John Daggett (:jtd) from comment #34) > Um, might have been nice to hold off on this until after the integration.. > Anyone with a patch to land will now need to integrate with this change... > Minor problem but one that greatly increases the chance of someone busting > the tree inadvertently. Ahh, sorry I was not aware - what integration is this? When is it expected to be complete?
(In reply to comment #36) > (In reply to John Daggett (:jtd) from comment #34) > > Um, might have been nice to hold off on this until after the integration.. > > Anyone with a patch to land will now need to integrate with this change... > > Minor problem but one that greatly increases the chance of someone busting > > the tree inadvertently. > > Ahh, sorry I was not aware - what integration is this? When is it expected to > be complete? I think John was talking about Nightly -> Aurora uplift. But I think landing this at any time would break people's patches. Landing them this way by just landing on one branch, and then have it backed out is even worse since some people (like me) would have rebased their stuff on top of the non-backed out version, and they would have to rebase again. At any rate, I think at the very least you should announce this on dev.platform to at least prevent people from being surprised.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #37) > (In reply to comment #36) > At any rate, I think at the very least you should announce this on > dev.platform to at least prevent people from being surprised. Last time (for EXPORTS I think?) there were complaints that I announced on dev.platform too soon since it was only in inbound and not yet in m-c. I guess there's no pleasing everybody :) In any case, I definitely would like to be able to land these changes with minimal impact. How best should we go about it?
(In reply to John Daggett (:jtd) from comment #34) > Um, might have been nice to hold off on this until after the integration.. > Anyone with a patch to land will now need to integrate with this change... > Minor problem but one that greatly increases the chance of someone busting > the tree inadvertently. Why are people adding or removing C++ files this close to an uplift? If anything, I'd think landing on this side of the uplift is easier! It's unfortunate this got backed out completely. It would have been much nicer if the fix could have been applied directly to inbound. Oh well.
(In reply to Gregory Szorc [:gps] from comment #39) > (In reply to John Daggett (:jtd) from comment #34) > > Um, might have been nice to hold off on this until after the integration.. > > Anyone with a patch to land will now need to integrate with this change... > > Minor problem but one that greatly increases the chance of someone busting > > the tree inadvertently. > > Why are people adding or removing C++ files this close to an uplift? If > anything, I'd think landing on this side of the uplift is easier! Is this a serious question? :-)
(In reply to Michael Shal [:mshal] from comment #38) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #37) > > (In reply to comment #36) > > At any rate, I think at the very least you should announce this on > > dev.platform to at least prevent people from being surprised. > > Last time (for EXPORTS I think?) there were complaints that I announced on > dev.platform too soon since it was only in inbound and not yet in m-c. I > guess there's no pleasing everybody :) > > In any case, I definitely would like to be able to land these changes with > minimal impact. How best should we go about it? We talked on IRC today, and I suggested the usual "close all branches, land on central, merge to inbound/birch, reopen" approach we take for these kinds of big changes. Usually a notice of a few days in advance on dev.platform is sufficient.
The patch in bug 861587 conditionally defines CPPSRCS based on the MAKECMDGOALS (to work around the silly way we pull in .deps based on MAKECMDGOALS). How should that be done in the new world?
I know people are always in a mad rush to land before an uplift. But, my impression was larger landings (e.g. new source files) are discouraged immediately before an uplift. I'm probably wrong.
(In reply to comment #43) > I know people are always in a mad rush to land before an uplift. But, my > impression was larger landings (e.g. new source files) are discouraged > immediately before an uplift. I'm probably wrong. We do not have any restriction on how many source files you can add or remove close to the uplift, since that has no relevance to the riskiness of the changes involved.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #44) > We do not have any restriction on how many source files you can add or > remove close to the uplift, since that has no relevance to the riskiness of > the changes involved. Maybe we should, especially now that we get closer to the address space exhaustion when linking windows PGO builds.
(In reply to comment #45) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #44) > > We do not have any restriction on how many source files you can add or > > remove close to the uplift, since that has no relevance to the riskiness of > > the changes involved. > > Maybe we should, especially now that we get closer to the address space > exhaustion when linking windows PGO builds. That problem is orthogonal. I was promised to find somebody who will be tasked to help fix that today. I'll follow-up on dev.platform once I know more.
What about GTEST_CPPSRCS? Will this continue to work?
Why not, while changing names, just get rid of CPP/C/AS/whatever in the variable name, and only have a SOURCES variable? The build system assumes that all CPPSRCS are .cpp or .cc, CSRCS are .c, ASFILES are .s or .asm, SSRCS are .S, CMSRCS are .m, CMMSRCS are .mm, etc. That is, we don't have .cpp files in CSRCS or ASFILES, and we don't have .asm files in CPPSRCS, etc. IOW, it's trivial to recreate CPPSRCS, CSRCS, and others from a single SOURCES variable, so why bother developers to have them do the separation that config/config.mk could do?
(In reply to Mike Hommey [:glandium] from comment #48) > Why not, while changing names, just get rid of CPP/C/AS/whatever in the > variable name, and only have a SOURCES variable? The build system assumes > that all CPPSRCS are .cpp or .cc, CSRCS are .c, ASFILES are .s or .asm, > SSRCS are .S, CMSRCS are .m, CMMSRCS are .mm, etc. That is, we don't have > .cpp files in CSRCS or ASFILES, and we don't have .asm files in CPPSRCS, > etc. IOW, it's trivial to recreate CPPSRCS, CSRCS, and others from a single > SOURCES variable, so why bother developers to have them do the separation > that config/config.mk could do? I think that's a terrific idea and something we should keep in mind when we refactor the sources definitions to be library or translation-unit centric.
Comment on attachment 746388 [details] [diff] [review] Move CPPSRCS to moz.build as CPP_SOURCES (comm-central) Note the calendar parts will have bitrot due to bug 677982 and it dependencies. Most of the moving happens from base/src to subdirectories of base/backend
Here is the unbitrotted patch. Note I haven't done anything to test this and am just supplying this patch for convenience.
Attachment #746388 - Attachment is obsolete: true
in xpcom/reflect/xptcall/src/md/win32/Makefile.in ASFILES is removed. I think it brokes the win64 build
Attached patch win64 fix (obsolete) — — Splinter Review
Yes, ASFILES for win64 are missing. The attached patch fixes the problem (tested with 64-bit mingw build).
Attachment #755284 - Flags: review?(mshal)
(In reply to Jacek Caban from comment #55) > Created attachment 755284 [details] [diff] [review] > win64 fix > > Yes, ASFILES for win64 are missing. The attached patch fixes the problem > (tested with 64-bit mingw build). Can you attach your patch to bug 877112 instead?
Depends on: 877112
Comment on attachment 755284 [details] [diff] [review] win64 fix Sure, done.
Attachment #755284 - Attachment is obsolete: true
Attachment #755284 - Flags: review?(mshal)
Depends on: 877139
This broke openbsd/amd64 builds (and maybe i386, checking) : :../../../../../../dist/include/xptcstubsdef.inc:252: first defined here ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_amd64_openbsd.o(.text+0x66): In function `PrepareAndDispatch': ../../../../../../dist/include/mozilla/mozalloc.h:213: multiple definition of `nsXPTCStubBase::Stub4()' ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.o(.text+0x18):/src/mozilla-central/xpcom/reflect/xptcall/src/md/ unix/xptcstubs_gcc_x86_unix.cpp:28: first defined here ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_amd64_openbsd.o(.text+0x6e): In function `PrepareAndDispatch': ../../../../../../dist/include/mozilla/mozalloc.h:213: multiple definition of `nsXPTCStubBase::Stub5()' ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.o(.text+0x20):/src/mozilla-central/xpcom/reflect/xptcall/src/md/ unix/xptcstubs_gcc_x86_unix.cpp:28: first defined here Should i file a followup ? I have no idea how this stubs work and if both files are supposed to be included. Btw, our 64 bit platform is amd64, not x86_64, if that matters for the "if CONFIG['OS_TEST'].find('86') != -1:" test, which i suppose is the problem here.
Blocks: 878171
(In reply to Landry Breuil (:gaston) from comment #59) > This broke openbsd/amd64 builds (and maybe i386, checking) : > > :../../../../../../dist/include/xptcstubsdef.inc:252: first defined here > ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_amd64_openbsd.o(. > text+0x66): In function `PrepareAndDispatch': > ../../../../../../dist/include/mozilla/mozalloc.h:213: multiple definition > of `nsXPTCStubBase::Stub4()' > ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.o(.text+0x18): > /src/mozilla-central/xpcom/reflect/xptcall/src/md/ > unix/xptcstubs_gcc_x86_unix.cpp:28: first defined here > ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_amd64_openbsd.o(. > text+0x6e): In function `PrepareAndDispatch': > ../../../../../../dist/include/mozilla/mozalloc.h:213: multiple definition > of `nsXPTCStubBase::Stub5()' > ../../xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.o(.text+0x20): > /src/mozilla-central/xpcom/reflect/xptcall/src/md/ > unix/xptcstubs_gcc_x86_unix.cpp:28: first defined here > > Should i file a followup ? I have no idea how this stubs work and if both > files are supposed to be included. > > Btw, our 64 bit platform is amd64, not x86_64, if that matters for the "if > CONFIG['OS_TEST'].find('86') != -1:" test, which i suppose is the problem > here. Ahh, sorry for breaking that. I'll try find a way to test it - in the meantime can you provide what OS_ARCH and OS_TEST are set in your build? And if you build a version prior to the CPPSRCS change, which files are compiled in that directory? The relevant if-statements in the Makefile.in are: ifneq (,$(filter NetBSD OpenBSD GNU,$(OS_ARCH))) ifeq (86,$(findstring 86,$(OS_TEST))) CPPSRCS := xptcinvoke_gcc_x86_unix.cpp xptcstubs_gcc_x86_unix.cpp endif endif which translates in moz.build as: if CONFIG['OS_ARCH'] in ('NetBSD', 'OpenBSD', 'GNU'): if CONFIG['OS_TEST'].find('86') != -1: And the OpenBSD/amd64 test in the Makefile.in is: ifeq ($(OS_ARCH)$(OS_TEST),OpenBSDx86_64) CPPSRCS := xptcinvoke_amd64_openbsd.cpp xptcstubs_amd64_openbsd.cpp endif which is converted to: if CONFIG['OS_ARCH'] == 'OpenBSD' and CONFIG['OS_TEST'] == 'x86_64': (the string concatenation in the Makefile is just a clever way of doing 'and' since make doesn't have binary logic). I don't know why the test is x86_64 instead of amd64 if that's what you actually want, but that's what it was in the Makefile.
it seems yuv convert does not compile on armv7l OS_TEST arch... http://hg.mozilla.org/mozilla-central/rev/6e45e9f62d21#l316.60 - strict condition, and it was http://hg.mozilla.org/mozilla-central/rev/6e45e9f62d21#l315.57
Have I mentioned that I hate OS_TEST? You should be able to change that to CONFIG['CPU_ARCH'] == 'arm'. r=me if that fixes it.
I've run this on try; suite/shell/src/Makefile.in seems to love fun here.
Attachment #754706 - Attachment is obsolete: true
Attachment #762371 - Flags: review?(mbanner)
Comment on attachment 762371 [details] [diff] [review] comm-central migration Review of attachment 762371 [details] [diff] [review]: ----------------------------------------------------------------- Looks good apart from a couple of minor indentation issues. ::: mail/components/migration/src/moz.build @@ +13,5 @@ > +] > + > +if CONFIG['OS_ARCH'] == 'WINNT': > + CPP_SOURCES += [ > + 'nsEudoraProfileMigrator.cpp', 10-space indentation? @@ +21,5 @@ > + ] > + > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': > + CPP_SOURCES += [ > + 'nsEudoraProfileMigrator.cpp', 6-space indentation?
Attachment #762371 - Flags: review?(mbanner) → review+
https://hg.mozilla.org/comm-central/rev/951512af3e49
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla24
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 886689
(In reply to Ted Mielczarek [:ted.mielczarek] (away June 25th-30th) from comment #62) > Have I mentioned that I hate OS_TEST? You should be able to change that to > CONFIG['CPU_ARCH'] == 'arm'. r=me if that fixes it. follow up by bug 886689
Blocks: 904572
This moves the remainder, except the more complex things that will require their own bug. I'm fine closing this bug when this patch lands.
Attachment #823894 - Flags: review?(mshal)
Assignee: mshal → mh+mozilla
Status: REOPENED → ASSIGNED
Comment on attachment 823894 [details] [diff] [review] part 3 - Move some more CPPSRCS to moz.build Review of attachment 823894 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/build/moz.build @@ +35,5 @@ > EXPORTS.ipc += [ > 'Nuwa.h', > ] > + SOURCES += [ > + 'Nuwa.cpp', This loses the OS_TARGET check
(In reply to :Ms2ger from comment #68) > > + SOURCES += [ > > + 'Nuwa.cpp', > > This loses the OS_TARGET check Which is pointless. First, because if it was in moz.build, it should apply to Nuwa.h too, which it doesn't currently ; second, because the test in Makefile.in is already wrong (it's for Android, where it should be for gonk) ; and third, because nuwa is only enabled on gonk, and whenever it will be enabled on other platforms, the file will be required.
Attachment #823894 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/48444fee320e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Blocks: xulinmozbuild
Depends on: 970031
Pushed by frgrahl@gmx.net: https://hg.mozilla.org/comm-central/rev/8cb34980d514 Part 2: Move CPPSRCS to moz.build as CPP_SOURCES; r=joey https://hg.mozilla.org/comm-central/rev/7c508dc68f5d Backed out changeset 8cb34980d514 https://hg.mozilla.org/comm-central/rev/664a157c798e Part 2: Move CPPSRCS to moz.build as CPP_SOURCES; r=joey CLOSED TREE
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: