1455095 - Changing an XPIDL file in an artifact build should produce an error

Status: RESOLVED DUPLICATE of bug 1455102

(Firefox Build System :: General, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 1438688 made it so that all XPT information gets compiled into the binary. This means that in an artifact build changing .idl files does not do anything, even though you'll see a new XPT file being built. As Gijs related on dev.firefox,it can be difficult to tell that this is going wrong, thanks to the forgiving nature of JS.

We should produce an error if you try to build an XPT file in an artifact build. I don't know what the best approach for that is.

Note that this only ever worked for JS to JS calls, and ideally those calls would not go through XPCOM. Potentially we could support dynamic loading of XPTs at startup time in artifact builds for JS to JS calls, but hopefully the effort would be better spent on removing them.

Comment 1

[Andrew McCreight [:mccr8]]

I can take a look at this.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

We should have stopped compiling xpidl in artifact builds in bug 1240134.

Comment 3

[Andrew McCreight [:mccr8]]

Hmm, yeah that's odd. I'll have to see what actually happens when you touch an XPIDL file in an artifact build.

xpcom/xpidl/Makefile.in contains this:

export::
	$(call py_action,process_install_manifest,--track install-xpidl.track $(DIST)/idl $(DEPTH)/_build_manifests/install/dist_idl)
	$(call SUBMAKE,xpidl,$(DEPTH)/config/makefiles/xpidl)

(Hmm the manifest stuff feels like it shouldn't be needed any more...)

But then in that other makefile, the xpidl phony rule is guarded by ifdef COMPILE_ENVIRONMENT.

Comment 4

[Kris Maglione [:kmag]]

I think the problem is less a matter of whether we try to compile them, and more a matter of us not detecting changes from the base artifact revision that require a full compile.

I'm not sure how easy a problem this is to fix, but we do know the base revision we pulled artifacts for, and which inputs can be built from an artifact build. So I think during the artifact install process, we need to check for changes in those files after the artifact revision against the list of compile-only inputs.

And, at that point, there should probably be two failure modes:

1) For changes to idl/webidl files, we should fail to build if there are *any* changes to the files, either local or remote, since the artifact revision. I occasionally run into failures where I pull remote changesets with interface changes that artifacts aren't available for yet, and it's not obvious why they fail unless you're pretty familiar with the specifics of artifact builds.

2) For changes to cpp/rust files, we should only fail if there are local changes (i.e., uncommitted or in draft changesets). It's pretty common to wind up building with artifacts for a slightly older commit than the head you just pulled, and unless there are interface changes, other binary changes shouldn't matter much. But if you changed a compile-only source yourself, you're probably expecting that change to be picked up.

Comment 5

[Andrew McCreight [:mccr8]]

Gijs, what steps were you taking that worked before, but don't work now?

Based on my understanding of what you were trying to do, I checked out a build from hg revision ef717c03ff54 (before my new XPT stuff landed). Then I did an artifact build (using |mach build|). Then, I edited nsIBlocklistService.idl to change the return type of getAddonBlocklistState() to be jsval and ran |mach build again|, but it did not rebuild any XPT files (which I think is what chmanchester was expecting in comment 2).

Comment 6

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Gijs, what steps were you taking that worked before, but don't work now?
> 
> Based on my understanding of what you were trying to do, I checked out a
> build from hg revision ef717c03ff54 (before my new XPT stuff landed). Then I
> did an artifact build (using |mach build|). Then, I edited
> nsIBlocklistService.idl to change the return type of
> getAddonBlocklistState() to be jsval and ran |mach build again|, but it did
> not rebuild any XPT files (which I think is what chmanchester was expecting
> in comment 2).

Yeah, I suspect that I was just wrong, and this hasn't worked for longer than that. Sorry for the confusion and extra effort. :-(

I tried to look into this myself (still kind of curious if this hasn't worked for 2 years without me realizing, or if e.g. adding new not-previously-in-idl methods/constants happens to work for JS-to-JS xpidl or what), before answering but so far today I've managed to spill tea on my mbp so that's MIA, on Windows it seems building artifacts off esr doesn't work, and I don't think chasing this more is likely to help...

I still think a fix for the bug as summarized would be helpful (and I think comment #4 would be a good way to handle this), but I no longer think it's a blocking/immediate regression.

Comment 7

[:Gijs (he/him)]

Potentially what's confused me is that if you run ./mach build faster on a non-artifact build, it *will* rebuild all interfaces.

Comment 8

[Andrew McCreight [:mccr8]]

Thanks for clarifying. I'll just dupe this to the general bug then.