Updates to the add-on SDK APIs don't propagate to the build

RESOLVED FIXED in mozilla21

Status

defect
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla21
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

I think due to how the copies work since the top-level directories of the modules never change we'll need a clobber whenever files are added or removed from the API source.
Posted patch quick n dirty (obsolete) — Splinter Review
I bet you have a better way to do this, right?
Attachment #709958 - Flags: review?(gps)
Comment on attachment 709958 [details] [diff] [review]
quick n dirty

Review of attachment 709958 [details] [diff] [review]:
-----------------------------------------------------------------

I should have caught this during initial review. Sorry about that. You'll need to list all files explicitly since directory mtime comparisons won't do what you want. Blame make.

Make has a $(wildcard) function that you can employ. However, the risk with $(wildcard) is accidentally pulling things in. I opt to explicitly list all files.

https://www.gnu.org/software/make/manual/make.html#Wildcard-Function

See /services/sync/Makefile.in for an example of a Makefile.in listing multiple JSMs in multiple directories (although not using $(wildcard)).
Attachment #709958 - Flags: review?(gps) → review-
Comment on attachment 709958 [details] [diff] [review]
quick n dirty

Review of attachment 709958 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, the r- was for doing it too quick and dirty. If the timeline is urgent, I could be convinced to r+. But, I'd insist on the follow-up bug and I probably wouldn't hear the end of it from ted and glandium :)
(In reply to Gregory Szorc [:gps] from comment #2)
> I should have caught this during initial review. Sorry about that. You'll
> need to list all files explicitly since directory mtime comparisons won't do
> what you want. Blame make.

Maintaining that is going to be a pain, why can't we just do this?

> Make has a $(wildcard) function that you can employ. However, the risk with
> $(wildcard) is accidentally pulling things in. I opt to explicitly list all
> files.

That doesn't recursively work I think
(In reply to Dave Townsend (:Mossop) from comment #4)
> (In reply to Gregory Szorc [:gps] from comment #2)
> > I should have caught this during initial review. Sorry about that. You'll
> > need to list all files explicitly since directory mtime comparisons won't do
> > what you want. Blame make.
> 
> Maintaining that is going to be a pain, why can't we just do this?

The |rm -rf| is essentially preventing no-op builds and slowing down the build. That is why I strongly prefer to not do this.

> > Make has a $(wildcard) function that you can employ. However, the risk with
> > $(wildcard) is accidentally pulling things in. I opt to explicitly list all
> > files.
> 
> That doesn't recursively work I think

Correct. make sucks for this kind of thing.

Long term, we could probably write a moz.build directive that says "take all files matching this wildcard in this directory and copy them here." We already have code for that in the packager. But that won't help you until bug 784841 lands.

If you wanted to invest a little bit of effort, you could write a (preferably Python) script that traverses the directory tree and writes out a .mk file. Your Makefile.in could run this script and then run make against its output. Done properly, we could probably generalize that for others to use. But, I won't make you do that.

Anyway, I'm not comfortable granting an exception to no-op builds. Please r? ted or glandium.
Blocks: 838716
Posted patch patch rev 1Splinter Review
This does it with an intermediate makefile generated from a python script. The script scans the source directory and generates an INSTALL_TARGET section for each directory.

The python script is currently hardcoded to only work for addon-sdk but it wouldn't be difficult to make it more generic in the future.
Attachment #709958 - Attachment is obsolete: true
Attachment #713125 - Flags: review?(gps)
An example file generated by the python script.
Assignee: nobody → dtownsend+bugmail
Comment on attachment 713125 [details] [diff] [review]
patch rev 1

Review of attachment 713125 [details] [diff] [review]:
-----------------------------------------------------------------

Please correct Python style nits before checking in. You may find https://bitbucket.org/tarek/flake8/overview useful for assessing your Python style. I mostly care about 4 space indent. The others are super nitpicky.

If you have a few minutes, please also attempt the copy_source.mk:: rule. If it works, I'd prefer that (mainly to avoid the extra make invocation).

::: addon-sdk/Makefile.in
@@ +12,5 @@
>  TEST_DIRS += test
>  
> +libs::
> +	$(PYTHON) $(srcdir)/copy_source.py $(topsrcdir) $(srcdir)/source/lib $(FINAL_TARGET)/modules/commonjs >copy_source.mk
> +	$(MAKE) -f copy_source.mk libs

I was thinking that we could alternatively create a rule for copy_source.mk. e.g.

copy_source.mk::
    $(PYTHON) ... > copy_source.mk

Then if you "include copy_source.mk" in your make file, I believe make will automagically see it has a rule and since it is a double colon rule it will always execute. Only after it has regenerated copy_source.mk will it include it.

This /might/ be slightly better from a make style convention and has the advantage that we avoid an extra make invocation. I also can't remember if $(MAKE) will pass down -j. I /think/ it does. If it doesn't, we should definitely change.

That being said, meh. This is good enough to land, IMO.

::: addon-sdk/copy_source.py
@@ +1,5 @@
> +# 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/.
> +
> +import os, sys

Nit: One import per line.

@@ +23,5 @@
> +
> +real_source = source_dir.replace('/', os.sep)
> +if not os.path.exists(real_source):
> +  print >> sys.stderr, "Error: Missing source file %s" % real_source
> +  sys.exit(1)

Nit: 4 space indent.

@@ +27,5 @@
> +  sys.exit(1)
> +elif not os.path.isdir(real_source):
> +  print >> sys.stderr, "Error: Source %s is not a directory" % real_source
> +  sys.exit(1)
> +for (dirpath, dirnames, filenames) in os.walk(real_source):

Nit: You don't need () for destructured assignment in Python.

@@ +28,5 @@
> +elif not os.path.isdir(real_source):
> +  print >> sys.stderr, "Error: Source %s is not a directory" % real_source
> +  sys.exit(1)
> +for (dirpath, dirnames, filenames) in os.walk(real_source):
> +  if len(filenames) == 0:

Nit: if not filenames (empty list is falsey)
Attachment #713125 - Flags: review?(gps) → review+
Landed with the python nits addressed. It looked like it attempted to do the include before using the rule to generate copy_source.mk when I attempted that so I left it for now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b817e00e439
Target Milestone: --- → mozilla21
Blocks: 841418
(In reply to Dave Townsend (:Mossop) from comment #9)
> It looked like it attempted to do the
> include before using the rule to generate copy_source.mk when I attempted
> that so I left it for now.

Interesting. I thought make was smart enough to handle that. Maybe if you used "-include". I dunno. Make is so much voodoo. Meh.
FWIW, the following works (with make -f thatfile.mk foo)

-include foo.mk

foo.mk:
        echo foo: > foo.mk
        echo \\techo foo >> foo.mk
Posted patch avoid make invocation (obsolete) — Splinter Review
Yeah that seems to work better and avoids needing to pass topsrcdir in to the script making it more generic, spinning this through try now.

One issue that this introduces, running "make -C addon-sdk" doesn't automatically run the libs targets as it does currently, is there a way to fix that?
Attachment #714043 - Flags: feedback?(mh+mozilla)
With the in-tree code, I'm seeing a number of new files created in the srcdir:

? addon-sdk/source/lib/sdk/.mkdir.done
? addon-sdk/source/lib/sdk/addon/.mkdir.done
? addon-sdk/source/lib/sdk/console/.mkdir.done
? addon-sdk/source/lib/sdk/content/.mkdir.done
? addon-sdk/source/lib/sdk/core/.mkdir.done
? addon-sdk/source/lib/sdk/deprecated/.mkdir.done
? addon-sdk/source/lib/sdk/deprecated/events/.mkdir.done
? addon-sdk/source/lib/sdk/deprecated/traits/.mkdir.done

Hopefully the new patch fixes this!
(In reply to Gregory Szorc [:gps] from comment #13)
> With the in-tree code, I'm seeing a number of new files created in the
> srcdir:
> 
> ? addon-sdk/source/lib/sdk/.mkdir.done
> ? addon-sdk/source/lib/sdk/addon/.mkdir.done
> ? addon-sdk/source/lib/sdk/console/.mkdir.done
> ? addon-sdk/source/lib/sdk/content/.mkdir.done
> ? addon-sdk/source/lib/sdk/core/.mkdir.done
> ? addon-sdk/source/lib/sdk/deprecated/.mkdir.done
> ? addon-sdk/source/lib/sdk/deprecated/events/.mkdir.done
> ? addon-sdk/source/lib/sdk/deprecated/traits/.mkdir.done
> 
> Hopefully the new patch fixes this!

This is because we need to clobber since dist/bin/commonjs/sdk is a symlink to the srcdir on some platforms and the makefile is attempting to copy stuff over itself there. It's harmless so I'm inclined to just leave it till the next thing that needs a clobber lands but I could land a CLOBBER update if you want?
https://hg.mozilla.org/mozilla-central/rev/3b817e00e439
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #714043 - Flags: feedback?(mh+mozilla) → feedback+
Depends on: 842341
On our SeaMonkey windows boxes we are still stuck with GNU make instead of pymake so we are getting errors like this:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1361344746.1361353569.23649.gz&fulltext=1#err0

make -C addon-sdk libs
make[5]: Entering directory `/e/builds/slave/c-cen-t-w32-dbg/build/objdir/mozilla/addon-sdk'
e:/builds/slave/c-cen-t-w32-dbg/build/objdir/mozilla/_virtualenv/Scripts/python.exe /e/builds/slave/c-cen-t-w32-dbg/build/mozilla/addon-sdk/copy_source.py /e/builds/slave/c-cen-t-w32-dbg/build/mozilla /e/builds/slave/c-cen-t-w32-dbg/build/mozilla/addon-sdk/source/lib ../dist/bin/modules/commonjs >copy_source.mk
make -f copy_source.mk libs
make[6]: Entering directory `/e/builds/slave/c-cen-t-w32-dbg/build/objdir/mozilla/addon-sdk'
make[6]: Leaving directory `/e/builds/slave/c-cen-t-w32-dbg/build/objdir/mozilla/addon-sdk'
make[5]: Leaving directory `/e/builds/slave/c-cen-t-w32-dbg/build/objdir/mozilla/addon-sdk'
e:/builds/slave/c-cen-t-w32-dbg/build/mozilla/config/baseconfig.mk:20: *** Windows-style srcdir being used with GNU make. Did you mean to run e:/builds/slave/c-cen-t-w32-dbg/build/mozilla/build/pymake/make.py instead? [see-also:     https://developer.mozilla.org/en/Gmake_vs._Pymake].  Stop.
NEXT ERROR make[5]: *** [libs] Error 2
(In reply to Philip Chee from comment #16)
> On our SeaMonkey windows boxes we are still stuck with GNU make instead of
> pymake so we are getting errors like this:

Does the most recent patch fix that?
Comment on attachment 714043 [details] [diff] [review]
avoid make invocation

This doesn't seem to be working at all :(
Attachment #714043 - Attachment is obsolete: true
We are very close to unsupporting GNU make on Windows. I don't think we should spend time working around things to make it work.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.