We think EXPORTS are the next target after XPIDLSRCS to move out of Makefile.in and into moz.build. So, how do we want to do this in a moz.build world? Currently, we do something like: --- # Copy files to the main headers directory. EXPORTS = foo.h bar.h # Copy files into the mozilla/dom subdirectory. EXPORTS_NAMESPACES = mozilla/dom EXPORTS_mozilla/dom = dom.h --- I'd like to not carry this over 1:1 because it requires trapping writes for dynamic variable names in the sandbox. Doable, but a little extra work. We have some options. Assuming we name the variable EXPORTS (we should consider other names!), here are some options: --- # 1. EXPORTS is a dict. # The main namespace is the special empty string key: EXPORTS[''] += ['foo.h', 'bar.h'] # Other namespaces are just keys in the dict: EXPORTS['mozilla'] += ['mozilla.h'] EXPORTS['mozilla/dom'] += ['dom.h'] --- # 2. EXPORTS is a class implementing accessor magic methods. # If we += on the instance, we magically intercept the operation and treat as # additions to the main namespace (via __iadd__): EXPORTS += ['foo.h', 'bar.h'] # The class behaves like a dictionary to allow assignment to any namespace: EXPORTS['mozilla'] += ['mozilla.h'] EXPORTS['mozilla/dom'] += ['dom.h'] # OR # The class has __getattr__ magic so attribute access retrieves namespaces: EXPORTS.mozilla += ['mozilla.h] EXPORTS.mozilla.dom += ['dom.h'] --- Anyone have any opinions?

(In reply to Gregory Szorc [:gps] from comment #0) > # 1. EXPORTS is a dict. This works for me, but I have no love for it. > # 2. EXPORTS is a class implementing accessor magic methods. Eh. Definitely not a fan of this. > # OR Though this actually looks reasonably nice.

My vote goes to EXPORTS += ['foo.h', 'bar.h'] EXPORTS.mozilla += ['mozilla.h] EXPORTS.mozilla.dom += ['dom.h']

I like that one as well. I think we're decided. If we were to describe this, we would say something like: EXPORTS can be treated as a list, and its contents are headers that will be copied to $(DIST)/include. It also allows setting arbitrary attributes on it that will be treated as lists of headers to install in a subdirectory under $(DIST)/include, named for the attribute name. Attributes may contain sub-attributes recursively to produce a nested directory structure.

Over to mshal.

Is there a better approach to what I have here with the "ExportVariableSandbox" and "ExportVariable" classes? (Or at least, better names? :) The actual conversion patch will be done later once this part is ironed out to avoid bitrot.

Comment on attachment 727217 [details] [diff] [review] Part 1: Support EXPORTS[_NAMESPACES] in moz.build Review of attachment 727217 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. Review comments mostly center around rigor and future reuse of this code. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +298,5 @@ > fh.write('PARALLEL_DIRS += %s\n' % > ' '.join(obj.parallel_external_make_dirs)) > > + def _process_exports(self, exports, backend_file): > + fh = backend_file.fh BackendMakeFile has a .write(), so you don't need to do this. @@ +299,5 @@ > ' '.join(obj.parallel_external_make_dirs)) > > + def _process_exports(self, exports, backend_file): > + fh = backend_file.fh > + if exports._name: Private variable access == bad. @@ +309,5 @@ > + for export in exports._exports: > + fh.write('%s += %s\n' % (export_name, export)) > + > + for subdir in exports._subdirs: > + self._process_exports(exports._subdirs[subdir], backend_file) I was going to say that for a first run at this, I would have been fine with the emitter creating the variables and not worrying about giving exports their own SandboxDerived class. But, now that you've done most of the work, terrific! ::: python/mozbuild/mozbuild/frontend/data.py @@ +145,5 @@ > + """ > + > + def __init__(self, sandbox, exports): > + SandboxDerived.__init__(self, sandbox) > + self.exports = exports The problem here is that we are forced to access private (_-prefixed) members of ExportVariable instances. Not cool. We either need to copy data into public attributes or expose APIs on ExportVariable. I think defining __iter__ on ExportVariable could go a long way. A recursive iterator would be even cooler. @@ +147,5 @@ > + def __init__(self, sandbox, exports): > + SandboxDerived.__init__(self, sandbox) > + self.exports = exports > + > +class ExportVariable(object): Any chance you could make this class generic and move it to utils.py? That's where we have our generic container classes today. And I have a feeling we'll have a need for a variation of this class in the future. @@ +161,5 @@ > + """ > + def __init__(self, name=""): > + self._exports = [] > + self._subdirs = {} > + self._name = name Since we'll have many of these class instances around, you may want to consider using __slots__. However, that has implications for the magic methods because __slots__ removes self.__dict__. It's funky. @@ +165,5 @@ > + self._name = name > + > + def __getattr__(self, name): > + if name.startswith('__'): > + return object.getattr(name) Why the double __? Isn't __getattr__ only called for attributes on which name lookup has already failed? Presumably this method just needs to validate the requested key looks like a valid namespace. If not, fall back to object.__getattr__(name) - not object.getattr(). That check might be a little complicated. We should also consider rejecting names that looks like namespaces but aren't valid. e.g. 'foo!' or 'foo bar'. regexp time? @@ +171,5 @@ > + if self._name: > + varname = '%s/%s' % (self._name, name) > + else: > + varname = name > + self._subdirs[name] = ExportVariable(varname) The whole 'name' bit may get removed from this class if we refactor to be generic. We can derive all that (including the paths) after moz.build execution - in the emitter. The Pythonic way to write "if x not in y: y[x] = z" is: thing = y.setdefault(x, z) e.g. self._subdirs.setdefault(name, ExportVariable(varname)) Keep in mind that the default value is evaluated before function call. So if it's expensive, that's a good reason to not use setdefault()! (Simple classes are cheap to create.) @@ +172,5 @@ > + varname = '%s/%s' % (self._name, name) > + else: > + varname = name > + self._subdirs[name] = ExportVariable(varname) > + return self._subdirs[name] You should also define __setattr__. Otherwise: >>> from mozbuild.frontend.data import ExportVariable >>> e = ExportVariable() >>> e.foo = 'bar' >>> e.foo 'bar' That's not right! @@ +175,5 @@ > + self._subdirs[name] = ExportVariable(varname) > + return self._subdirs[name] > + > + def __iadd__(self, other): > + self._exports += other We should validate other is the proper type (presumably a list) and that its elements are of the proper type (presumably unicode - assuming Python 2.7). Also, I /think/ list's += isn't optimal. I /think/ it is better to .extend(). We only use += in the moz.build files themselves for readability. I could be wrong about this. ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +87,5 @@ > > + exports = sandbox.get('EXPORTS') > + if exports: > + export_sandbox = ExportVariableSandbox(sandbox, exports) > + yield export_sandbox I would perform path construction in the emitter, not at variable assignment time. That's what the emitter is for. ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py @@ +157,5 @@ > + > + self.assertEqual(lines, [ > + 'MOZBUILD_DERIVED := 1', > + 'NO_MAKEFILE_RULE := 1', > + 'NO_SUBMAKEFILES_RULE := 1', Keep reminding me that we need better test helpers to only read variables we care about. It's going to be annoying when we change the set of default variables and have to update N tests. ::: python/mozbuild/mozbuild/test/frontend/data/exports/moz.build @@ +6,5 @@ > +EXPORTS.mozilla += ['mozilla1.h'] > +EXPORTS.mozilla += ['mozilla2.h'] > +EXPORTS.mozilla.dom += ['dom1.h'] > +EXPORTS.mozilla.dom += ['dom2.h', 'dom3.h'] > +EXPORTS.mozilla.gfx += ['gfx.h'] We also need tests for error conditions: EXPORTS += 'foo' EXPORTS.foo = 'for' EXPORTS.foo = ['foo'] EXPORTS += [True] del EXPORTS.foo You can probably do these inline - without moz.build files - by using sandbox.exec_source(). See test_sandbox.py.

(In reply to Gregory Szorc [:gps] from comment #6) > Comment on attachment 727217 [details] [diff] [review] > Part 1: Support EXPORTS[_NAMESPACES] in moz.build > > Review of attachment 727217 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is looking good. Review comments mostly center around rigor and future > reuse of this code. > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +298,5 @@ > > fh.write('PARALLEL_DIRS += %s\n' % > > ' '.join(obj.parallel_external_make_dirs)) > > > > + def _process_exports(self, exports, backend_file): > > + fh = backend_file.fh > > BackendMakeFile has a .write(), so you don't need to do this. Ok, done. Should _process_directory_traversal be changed too? Or is the local fh there just to save typing since it's used many times? > > @@ +299,5 @@ > > ' '.join(obj.parallel_external_make_dirs)) > > > > + def _process_exports(self, exports, backend_file): > > + fh = backend_file.fh > > + if exports._name: > > Private variable access == bad. > > > ::: python/mozbuild/mozbuild/frontend/data.py > @@ +145,5 @@ > > + """ > > + > > + def __init__(self, sandbox, exports): > > + SandboxDerived.__init__(self, sandbox) > > + self.exports = exports > > The problem here is that we are forced to access private (_-prefixed) > members of ExportVariable instances. Not cool. We either need to copy data > into public attributes or expose APIs on ExportVariable. I think defining > __iter__ on ExportVariable could go a long way. A recursive iterator would > be even cooler. I've added an __iter__ to iterate over the exports list. The backend still has to get the namespace (using get_namespace() now), and iterate over the sub-directories (using get_subdirs()). I've left this up to the backend, since I think others (like a tup backend) will probably want to handle these differently from make. > > @@ +147,5 @@ > > + def __init__(self, sandbox, exports): > > + SandboxDerived.__init__(self, sandbox) > > + self.exports = exports > > + > > +class ExportVariable(object): > > Any chance you could make this class generic and move it to utils.py? That's > where we have our generic container classes today. And I have a feeling > we'll have a need for a variation of this class in the future. You mean config/utils.py? I don't really see how it would be used outside of mozbuild - can you elaborate on that? > > @@ +161,5 @@ > > + """ > > + def __init__(self, name=""): > > + self._exports = [] > > + self._subdirs = {} > > + self._name = name > > Since we'll have many of these class instances around, you may want to > consider using __slots__. However, that has implications for the magic > methods because __slots__ removes self.__dict__. It's funky. Ok, it uses __slots__ now. This also required adding a __setattr__ function. > > @@ +165,5 @@ > > + self._name = name > > + > > + def __getattr__(self, name): > > + if name.startswith('__'): > > + return object.getattr(name) > > Why the double __? Isn't __getattr__ only called for attributes on which > name lookup has already failed? Presumably this method just needs to > validate the requested key looks like a valid namespace. If not, fall back > to object.__getattr__(name) - not object.getattr(). __getattr__ is called with name == __deepcopy__, __getnewargs__, __getstate__, __dict__, and __setstate__. The startswith() was just used as a catch-all for these variables. I don't have an explanation for why __getattr__ is called with these parameters, though - I just added a 'print name' in __getattr__ to see what was going on. I've changed it to use object.__getattr__ now. > > That check might be a little complicated. We should also consider rejecting > names that looks like namespaces but aren't valid. e.g. 'foo!' or 'foo bar'. > regexp time? > > @@ +171,5 @@ > > + if self._name: > > + varname = '%s/%s' % (self._name, name) > > + else: > > + varname = name > > + self._subdirs[name] = ExportVariable(varname) > > The whole 'name' bit may get removed from this class if we refactor to be > generic. We can derive all that (including the paths) after moz.build > execution - in the emitter. > > The Pythonic way to write "if x not in y: y[x] = z" is: > > thing = y.setdefault(x, z) > > e.g. > > self._subdirs.setdefault(name, ExportVariable(varname)) > > Keep in mind that the default value is evaluated before function call. So if > it's expensive, that's a good reason to not use setdefault()! (Simple > classes are cheap to create.) Ok, it uses setdefault() now. > > @@ +172,5 @@ > > + varname = '%s/%s' % (self._name, name) > > + else: > > + varname = name > > + self._subdirs[name] = ExportVariable(varname) > > + return self._subdirs[name] > > You should also define __setattr__. Otherwise: > > >>> from mozbuild.frontend.data import ExportVariable > >>> e = ExportVariable() > >>> e.foo = 'bar' > >>> e.foo > 'bar' > > That's not right! Ok, I've added __setattr__ (required for __slots__ anyway), and added some tests for using a regular assignment. > > @@ +175,5 @@ > > + self._subdirs[name] = ExportVariable(varname) > > + return self._subdirs[name] > > + > > + def __iadd__(self, other): > > + self._exports += other > > We should validate other is the proper type (presumably a list) and that its > elements are of the proper type (presumably unicode - assuming Python 2.7). > > Also, I /think/ list's += isn't optimal. I /think/ it is better to > .extend(). We only use += in the moz.build files themselves for readability. > I could be wrong about this. I ran some tests, and it seems += is ever so slightly faster, unless you cache the .extend lookup. I don't think one is better than another here. > > ::: python/mozbuild/mozbuild/frontend/emitter.py > @@ +87,5 @@ > > > > + exports = sandbox.get('EXPORTS') > > + if exports: > > + export_sandbox = ExportVariableSandbox(sandbox, exports) > > + yield export_sandbox > > I would perform path construction in the emitter, not at variable assignment > time. That's what the emitter is for. I'm not sure what you mean here - can you elaborate a bit? > > ::: python/mozbuild/mozbuild/test/frontend/data/exports/moz.build > @@ +6,5 @@ > > +EXPORTS.mozilla += ['mozilla1.h'] > > +EXPORTS.mozilla += ['mozilla2.h'] > > +EXPORTS.mozilla.dom += ['dom1.h'] > > +EXPORTS.mozilla.dom += ['dom2.h', 'dom3.h'] > > +EXPORTS.mozilla.gfx += ['gfx.h'] > > We also need tests for error conditions: > > EXPORTS += 'foo' > EXPORTS.foo = 'for' > EXPORTS.foo = ['foo'] > EXPORTS += [True] > del EXPORTS.foo > > You can probably do these inline - without moz.build files - by using > sandbox.exec_source(). See test_sandbox.py. I've added a bunch of tests for these error conditions. What is wrong with: EXPORTS.foo = ['foo'] ? Currently it just sets the foo exports to ['foo'] (overwriting any previous EXPORTS.foo), rather than giving an error. Also, the del test already fails with an AttributeError because we don't actually have a 'foo' attribute. Are you expecting that to be caught by something else or is that ok?

(In reply to Michael Shal [:mshal] from comment #7) > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > > @@ +298,5 @@ > > > fh.write('PARALLEL_DIRS += %s\n' % > > > ' '.join(obj.parallel_external_make_dirs)) > > > > > > + def _process_exports(self, exports, backend_file): > > > + fh = backend_file.fh > > > > BackendMakeFile has a .write(), so you don't need to do this. > > Ok, done. Should _process_directory_traversal be changed too? Or is the > local fh there just to save typing since it's used many times? Yes, it's just an alias to save typing. > > > > @@ +147,5 @@ > > > + def __init__(self, sandbox, exports): > > > + SandboxDerived.__init__(self, sandbox) > > > + self.exports = exports > > > + > > > +class ExportVariable(object): > > > > Any chance you could make this class generic and move it to utils.py? That's > > where we have our generic container classes today. And I have a feeling > > we'll have a need for a variation of this class in the future. > > You mean config/utils.py? I don't really see how it would be used outside of > mozbuild - can you elaborate on that? I mean mozbuild.util. I think we'll have other uses of this class within moz.build files. > > > > @@ +165,5 @@ > > > + self._name = name > > > + > > > + def __getattr__(self, name): > > > + if name.startswith('__'): > > > + return object.getattr(name) > > > > Why the double __? Isn't __getattr__ only called for attributes on which > > name lookup has already failed? Presumably this method just needs to > > validate the requested key looks like a valid namespace. If not, fall back > > to object.__getattr__(name) - not object.getattr(). > > __getattr__ is called with name == __deepcopy__, __getnewargs__, > __getstate__, __dict__, and __setstate__. The startswith() was just used as > a catch-all for these variables. I don't have an explanation for why > __getattr__ is called with these parameters, though - I just added a 'print > name' in __getattr__ to see what was going on. > > I've changed it to use object.__getattr__ now. Python internally is looking for these magic attributes to determine what capabilities the class or instance has. Don't ask me how all of them work - I don't know! > > ::: python/mozbuild/mozbuild/frontend/emitter.py > > @@ +87,5 @@ > > > > > > + exports = sandbox.get('EXPORTS') > > > + if exports: > > > + export_sandbox = ExportVariableSandbox(sandbox, exports) > > > + yield export_sandbox > > > > I would perform path construction in the emitter, not at variable assignment > > time. That's what the emitter is for. > > I'm not sure what you mean here - can you elaborate a bit? I think the data structure exposed to moz.build files should be a dumb container. Paths will come into the picture after evaluation, when that data structure is processed by the emitter. > > > > ::: python/mozbuild/mozbuild/test/frontend/data/exports/moz.build > > @@ +6,5 @@ > > > +EXPORTS.mozilla += ['mozilla1.h'] > > > +EXPORTS.mozilla += ['mozilla2.h'] > > > +EXPORTS.mozilla.dom += ['dom1.h'] > > > +EXPORTS.mozilla.dom += ['dom2.h', 'dom3.h'] > > > +EXPORTS.mozilla.gfx += ['gfx.h'] > > > > We also need tests for error conditions: > > > > EXPORTS += 'foo' > > EXPORTS.foo = 'for' > > EXPORTS.foo = ['foo'] > > EXPORTS += [True] > > del EXPORTS.foo > > > > You can probably do these inline - without moz.build files - by using > > sandbox.exec_source(). See test_sandbox.py. > > I've added a bunch of tests for these error conditions. What is wrong with: > > EXPORTS.foo = ['foo'] Nothing? It depends if you wish to allow direct assignment or whether you want to force appending on everyone. > Also, the del test already fails with an AttributeError because we don't > actually have a 'foo' attribute. Are you expecting that to be caught by > something else or is that ok? I was thinking we may want to disallow deletions completely! If we allow them, we should at least have a test that verifies the del does the right thing, including in the case where the entry does not exist.

Comment on attachment 729176 [details] [diff] [review] Bug 846634 - Part 1: Support EXPORTS[_NAMESPACES] in moz.build Review of attachment 729176 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +306,5 @@ > + else: > + export_name = 'EXPORTS' > + > + for s in exports: > + backend_file.write('%s += %s\n' % (export_name, s)) for s in sorted(exports) Otherwise, if the Python is using a random seed for dict key order, the order will not be consistent, the file contents will change, and a rebuild will be triggered. @@ +308,5 @@ > + > + for s in exports: > + backend_file.write('%s += %s\n' % (export_name, s)) > + > + for subdir in exports.get_subdirs(): Ditto. ::: python/mozbuild/mozbuild/frontend/data.py @@ +173,5 @@ > + def get_subdirs(self): > + subdir_list = [] > + for k in self._subdirs: > + subdir_list.append(self._subdirs[k]) > + return subdir_list return self._subdirs.values() @@ +177,5 @@ > + return subdir_list > + > + def __iter__(self): > + for export in self._exports: > + yield export return iter(self._exports) @@ +182,5 @@ > + > + def __setattr__(self, name, value): > + if name in self.__slots__: > + object.__setattr__(self, name, value) > + else: if name in self.__slots__: return object.__setattr__(self, name, value) # No need for else! @@ +208,5 @@ > + > + def _check_list(self, value): > + if isinstance(value, list): > + for v in value: > + if not isinstance(v, basestring): basestring isn't in Python 3. But meh. See "str_type" in python/mach/mach/config.py for how to do this right. It's painful, I know. @@ +211,5 @@ > + for v in value: > + if not isinstance(v, basestring): > + raise Exception( > + 'Expected list of strings in EXPORTS, not an element ' > + 'of %s' % type(v)) ValueError is more appropriate than Exception. Ideally, we'd hook this ValueError up to the pretty error formatting. However, that would require you to emit a ValueError with special arguments and for additional changes to the sandbox. I'm not sure it's worth pursuing at this juncture. @@ +214,5 @@ > + 'Expected list of strings in EXPORTS, not an element ' > + 'of %s' % type(v)) > + else: > + raise Exception( > + 'Expected a list to be set in EXPORTS, not %s' % type(value)) Style nit: Use early return whenever possible. e.g. def _check_list(self, value): if not isinstance(value, list): raise ValueError('Expected a list to be set in EXPORTS, not %s' % type(value)) # There's no need to indent down here! ::: python/mozbuild/mozbuild/frontend/emitter.py @@ +89,5 @@ > > + exports = sandbox.get('EXPORTS') > + if exports: > + export_sandbox = ExportVariableSandbox(sandbox, exports) > + yield export_sandbox Just yield it. No need for an additional variable. ::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py @@ +349,5 @@ > + > + with self.assertRaises(SandboxExecutionError) as se: > + sandbox.exec_source('EXPORTS += [True]', 'foo.py') > + > + self.assertEqual(se.exception.exc_type, Exception) It would be nice to see more thorough checking that the encountered error matches the error you expected. But, something *is* better than nothing. @@ +362,5 @@ > + > + with self.assertRaises(SandboxExecutionError) as se: > + sandbox.exec_source(source, 'foo.py') > + > + self.assertEqual(se.exception.exc_type, AttributeError) Oh, so we do disallow deletes!

(In reply to Gregory Szorc [:gps] from comment #10) > Comment on attachment 729176 [details] [diff] [review] > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +306,5 @@ > > + else: > > + export_name = 'EXPORTS' > > + > > + for s in exports: > > + backend_file.write('%s += %s\n' % (export_name, s)) > > for s in sorted(exports) > > Otherwise, if the Python is using a random seed for dict key order, the > order will not be consistent, the file contents will change, and a rebuild > will be triggered. This was actually iterating on a list, and I think we probably want to preserve the order given in the moz.build file. Although it doesn't matter for EXPORTS, another consumer of this class may want to keep an explicit ordering. (ie: if we were to use this class for listing libraries to link, we'd have to maintain the order from moz.build) > > @@ +308,5 @@ > > + > > + for s in exports: > > + backend_file.write('%s += %s\n' % (export_name, s)) > > + > > + for subdir in exports.get_subdirs(): > > Ditto. Yeah, this part should be sorted. > > ::: python/mozbuild/mozbuild/frontend/data.py > @@ +173,5 @@ > > + def get_subdirs(self): > > + subdir_list = [] > > + for k in self._subdirs: > > + subdir_list.append(self._subdirs[k]) > > + return subdir_list > > return self._subdirs.values() This is now get_children(), and we use both the key and value in the recursive make backend because the paths have been moved out of this class, so it just does 'return self._children' > > @@ +177,5 @@ > > + return subdir_list > > + > > + def __iter__(self): > > + for export in self._exports: > > + yield export > > return iter(self._exports) I ended up removing __iter__ and replacing it with get_strings() - it was a bit confusing to be able to iterate on the object, since in the backend we need to iterate on both the strings (eg: the export headers) and the children (eg: the sub-directories). Does 'return iter(self._strings)' do anything better than 'return self._strings'? > > @@ +182,5 @@ > > + > > + def __setattr__(self, name, value): > > + if name in self.__slots__: > > + object.__setattr__(self, name, value) > > + else: > > if name in self.__slots__: > return object.__setattr__(self, name, value) > > # No need for else! Without the else, we can't use '=', only '+='. Is that something you want to enforce? > > @@ +208,5 @@ > > + > > + def _check_list(self, value): > > + if isinstance(value, list): > > + for v in value: > > + if not isinstance(v, basestring): > > basestring isn't in Python 3. But meh. See "str_type" in > python/mach/mach/config.py for how to do this right. It's painful, I know. Doh! I copied the bits from config.py > > @@ +211,5 @@ > > + for v in value: > > + if not isinstance(v, basestring): > > + raise Exception( > > + 'Expected list of strings in EXPORTS, not an element ' > > + 'of %s' % type(v)) > > ValueError is more appropriate than Exception. > > Ideally, we'd hook this ValueError up to the pretty error formatting. > However, that would require you to emit a ValueError with special arguments > and for additional changes to the sandbox. I'm not sure it's worth pursuing > at this juncture. Do you still want to consider ValueError now that the code is in mozbuild/util.py? I've just left it as Exception for now. > > @@ +214,5 @@ > > + 'Expected list of strings in EXPORTS, not an element ' > > + 'of %s' % type(v)) > > + else: > > + raise Exception( > > + 'Expected a list to be set in EXPORTS, not %s' % type(value)) > > Style nit: Use early return whenever possible. e.g. > > def _check_list(self, value): > if not isinstance(value, list): > raise ValueError('Expected a list to be set in EXPORTS, not %s' % > type(value)) > > # There's no need to indent down here! Ok, I like your version better :) > > ::: python/mozbuild/mozbuild/frontend/emitter.py > @@ +89,5 @@ > > > > + exports = sandbox.get('EXPORTS') > > + if exports: > > + export_sandbox = ExportVariableSandbox(sandbox, exports) > > + yield export_sandbox > > Just yield it. No need for an additional variable. Ok. > > ::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py > @@ +349,5 @@ > > + > > + with self.assertRaises(SandboxExecutionError) as se: > > + sandbox.exec_source('EXPORTS += [True]', 'foo.py') > > + > > + self.assertEqual(se.exception.exc_type, Exception) > > It would be nice to see more thorough checking that the encountered error > matches the error you expected. But, something *is* better than nothing. I've added checking of the Exception string. Does that work or is there something better to check? > > @@ +362,5 @@ > > + > > + with self.assertRaises(SandboxExecutionError) as se: > > + sandbox.exec_source(source, 'foo.py') > > + > > + self.assertEqual(se.exception.exc_type, AttributeError) > > Oh, so we do disallow deletes! Well, not really - it just happens that 'del EXPORTS.foo' doesn't call __getattr__('foo'), so as far as 'del' is concerned, there is no foo to delete and we get an AttributeError. We only have the _strings and _children attributes. If not-deleting things is something you want to enforce in moz.build, it might make sense to enforce that more globally. Is there a way to do that? Or just by reviewing moz.build changes? :)

There's also collections.OrderedDict if you want it, it iterates in the order that keys were inserted.

(In reply to Michael Shal [:mshal] from comment #11) > > @@ +177,5 @@ > > > + return subdir_list > > > + > > > + def __iter__(self): > > > + for export in self._exports: > > > + yield export > > > > return iter(self._exports) > > I ended up removing __iter__ and replacing it with get_strings() - it was a > bit confusing to be able to iterate on the object, since in the backend we > need to iterate on both the strings (eg: the export headers) and the > children (eg: the sub-directories). Does 'return iter(self._strings)' do > anything better than 'return self._strings'? __iter__ (or any method that returns an iterator) should return an object that conforms to the "iterator object" specification. See http://docs.python.org/2/library/stdtypes.html#iterator-types. You *may* get lucky with some built-in types already conforming to the iterator protocol. And, Python may automagically obtain the iterator for an object defining __iter__. It's light magic. If the method is supposed to return an iterator object, I always return an explicit iterator, just to be explicit. > > > > @@ +182,5 @@ > > > + > > > + def __setattr__(self, name, value): > > > + if name in self.__slots__: > > > + object.__setattr__(self, name, value) > > > + else: > > > > if name in self.__slots__: > > return object.__setattr__(self, name, value) > > > > # No need for else! > > Without the else, we can't use '=', only '+='. Is that something you want to > enforce? I /think/ I meant you should use early return. e.g. if foo: return True # Remaining code. Instead of: if foo: return True else: # Remaining code. This often results in cleaner and easier to read code because more code is at the base indentation and because there are fewer conditional branches to wrap your head around. > > @@ +208,5 @@ > > > + > > > + def _check_list(self, value): > > > + if isinstance(value, list): > > > + for v in value: > > > + if not isinstance(v, basestring): > > > > basestring isn't in Python 3. But meh. See "str_type" in > > python/mach/mach/config.py for how to do this right. It's painful, I know. > > Doh! I copied the bits from config.py I don't pretend that all the code I write is Python 3 compatible :) Just know we'll need to cross this bridge eventually and the more you can make compatible today the easier your life will be later. I strongly encourage new code to be dual 2.7/3.3 compatible. But, I'm not enforcing it. > > @@ +211,5 @@ > > > + for v in value: > > > + if not isinstance(v, basestring): > > > + raise Exception( > > > + 'Expected list of strings in EXPORTS, not an element ' > > > + 'of %s' % type(v)) > > > > ValueError is more appropriate than Exception. > > > > Ideally, we'd hook this ValueError up to the pretty error formatting. > > However, that would require you to emit a ValueError with special arguments > > and for additional changes to the sandbox. I'm not sure it's worth pursuing > > at this juncture. > > Do you still want to consider ValueError now that the code is in > mozbuild/util.py? I've just left it as Exception for now. If you assign the wrong type to a value, you should raise ValueError. That's what ValueError is for. http://docs.python.org/2/library/exceptions.html#module-exceptions Generally speaking, it's a good idea to never raise Exception directly. However, most (including myself) are lazy and reuse Exception as a general failure class. > > ::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py > > @@ +349,5 @@ > > > + > > > + with self.assertRaises(SandboxExecutionError) as se: > > > + sandbox.exec_source('EXPORTS += [True]', 'foo.py') > > > + > > > + self.assertEqual(se.exception.exc_type, Exception) > > > > It would be nice to see more thorough checking that the encountered error > > matches the error you expected. But, something *is* better than nothing. > > I've added checking of the Exception string. Does that work or is there > something better to check? That's good enough! > > @@ +362,5 @@ > > > + > > > + with self.assertRaises(SandboxExecutionError) as se: > > > + sandbox.exec_source(source, 'foo.py') > > > + > > > + self.assertEqual(se.exception.exc_type, AttributeError) > > > > Oh, so we do disallow deletes! > > Well, not really - it just happens that 'del EXPORTS.foo' doesn't call > __getattr__('foo'), so as far as 'del' is concerned, there is no foo to > delete and we get an AttributeError. We only have the _strings and _children > attributes. > > If not-deleting things is something you want to enforce in moz.build, it > might make sense to enforce that more globally. Is there a way to do that? > Or just by reviewing moz.build changes? :) Since we're not supporting explicit delete and since this behavior is dependent on an implementation detail (albeit one with some test coverage), let's just make it official by implementing __delattr__ and having it always raise. What should it raise? Good question. AttributeError doesn't seem right. I'm not sure if there is a built-in exception that is. Maybe just overload Exception or roll your own child class.

Comment on attachment 730325 [details] [diff] [review] Bug 846634 - Part 1: Support EXPORTS[_NAMESPACES] in moz.build Review of attachment 730325 [details] [diff] [review]: ----------------------------------------------------------------- This is nearly an r+. I'm still looking at things in util.py. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +309,5 @@ > + > + # Iterate over the list of export filenames, printing out an EXPORTS > + # declaration for each. > + for s in strings: > + backend_file.write('%s += %s\n' % (export_name, s)) Possible performance concern: writing out separate variable assignments will surely take longer to parse than a single assignment with space-delimited filenames. Will will be able to see a difference? I don't know. This file is read by machines, not humans. So I'm inclined to say not to take the risk and just print a single line. ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py @@ +152,5 @@ > in the future. > """), > > + 'EXPORTS': (HierarchicalStringList, HierarchicalStringList(), > + """List of files to be exported, and in which subdirectories. Let's mention "include files" or similar somewhere in the description. ::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py @@ +317,5 @@ > + > + with self.assertRaises(SandboxExecutionError) as se: > + sandbox.exec_source('EXPORTS += "foo.h"', 'foo.py') > + > + self.assertEqual(se.exception.exc_type, Exception) self.assertIsInstance(se.exception.exc_type, Exception) @@ +319,5 @@ > + sandbox.exec_source('EXPORTS += "foo.h"', 'foo.py') > + > + self.assertEqual(se.exception.exc_type, Exception) > + self.assertEqual(str(se.exception.exc_value), > + "Expected a list of strings, not <type 'unicode'>") Py3 warning: This will read "<type 'str'>" @@ +371,5 @@ > + > + with self.assertRaises(SandboxExecutionError) as se: > + sandbox.exec_source(source, 'foo.py') > + > + self.assertEqual(se.exception.exc_type, AttributeError) So, all these tests are really just basic tests for HierarchicalStringList and aren't EXPORTS specific. It isn't really sandbox specific either. Could you add the basic HierarchicalStringList tests to test_utils.py? Also, where are all the tests showing that proper assignments work? I guess we have most of it in test_emitter.py, but it would be really nice to consolidate the tests for HierarchicalStringList as a basic type. ::: python/mozbuild/mozbuild/util.py @@ +15,5 @@ > > from StringIO import StringIO > > +if sys.version_info[0] == 3: > + from configparser import RawConfigParser Why do you import RawConfigParser?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13) > There's also collections.OrderedDict if you want it, it iterates in the > order that keys were inserted. OrderedDict sounds good if we want to automatically prune duplicates (like EXPORTS += ['foo.h', 'foo.h'] becomes just ['foo.h']). Otherwise it should stay as a list. I don't know if that's something we want to do or not -- gps?

I think we ought to just error if someone puts duplicated files into EXPORTS. We've been leaning towards strictness in moz.build files.

(In reply to Gregory Szorc [:gps] from comment #14) > (In reply to Michael Shal [:mshal] from comment #11) > > > @@ +177,5 @@ > > > > + return subdir_list > > > > + > > > > + def __iter__(self): > > > > + for export in self._exports: > > > > + yield export > > > > > > return iter(self._exports) > > > > I ended up removing __iter__ and replacing it with get_strings() - it was a > > bit confusing to be able to iterate on the object, since in the backend we > > need to iterate on both the strings (eg: the export headers) and the > > children (eg: the sub-directories). Does 'return iter(self._strings)' do > > anything better than 'return self._strings'? > > __iter__ (or any method that returns an iterator) should return an object > that conforms to the "iterator object" specification. See > http://docs.python.org/2/library/stdtypes.html#iterator-types. You *may* get > lucky with some built-in types already conforming to the iterator protocol. > And, Python may automagically obtain the iterator for an object defining > __iter__. It's light magic. If the method is supposed to return an iterator > object, I always return an explicit iterator, just to be explicit. Hmm, after reading through that I'm still not sure if this needs to be changed. Right now there are still 2 get functions, one for the list of strings at that level, and one for the children, since we need to iterate over both. > > > > > > > @@ +182,5 @@ > > > > + > > > > + def __setattr__(self, name, value): > > > > + if name in self.__slots__: > > > > + object.__setattr__(self, name, value) > > > > + else: > > > > > > if name in self.__slots__: > > > return object.__setattr__(self, name, value) > > > > > > # No need for else! > > > > Without the else, we can't use '=', only '+='. Is that something you want to > > enforce? > > I /think/ I meant you should use early return. e.g. > > if foo: > return True > > # Remaining code. > > Instead of: > > if foo: > return True > else: > # Remaining code. > > This often results in cleaner and easier to read code because more code is > at the base indentation and because there are fewer conditional branches to > wrap your head around. Ahh, that makes more sense :). Done. > > > > @@ +208,5 @@ > > > > + > > > > + def _check_list(self, value): > > > > + if isinstance(value, list): > > > > + for v in value: > > > > + if not isinstance(v, basestring): > > > > > > basestring isn't in Python 3. But meh. See "str_type" in > > > python/mach/mach/config.py for how to do this right. It's painful, I know. > > > > Doh! I copied the bits from config.py > > I don't pretend that all the code I write is Python 3 compatible :) Just > know we'll need to cross this bridge eventually and the more you can make > compatible today the easier your life will be later. I strongly encourage > new code to be dual 2.7/3.3 compatible. But, I'm not enforcing it. Ok, good to know. > > > > @@ +211,5 @@ > > > > + for v in value: > > > > + if not isinstance(v, basestring): > > > > + raise Exception( > > > > + 'Expected list of strings in EXPORTS, not an element ' > > > > + 'of %s' % type(v)) > > > > > > ValueError is more appropriate than Exception. > > > > > > Ideally, we'd hook this ValueError up to the pretty error formatting. > > > However, that would require you to emit a ValueError with special arguments > > > and for additional changes to the sandbox. I'm not sure it's worth pursuing > > > at this juncture. > > > > Do you still want to consider ValueError now that the code is in > > mozbuild/util.py? I've just left it as Exception for now. > > If you assign the wrong type to a value, you should raise ValueError. That's > what ValueError is for. > http://docs.python.org/2/library/exceptions.html#module-exceptions > > Generally speaking, it's a good idea to never raise Exception directly. > However, most (including myself) are lazy and reuse Exception as a general > failure class. These are now ValueErrors. > > > > @@ +362,5 @@ > > > > + > > > > + with self.assertRaises(SandboxExecutionError) as se: > > > > + sandbox.exec_source(source, 'foo.py') > > > > + > > > > + self.assertEqual(se.exception.exc_type, AttributeError) > > > > > > Oh, so we do disallow deletes! > > > > Well, not really - it just happens that 'del EXPORTS.foo' doesn't call > > __getattr__('foo'), so as far as 'del' is concerned, there is no foo to > > delete and we get an AttributeError. We only have the _strings and _children > > attributes. > > > > If not-deleting things is something you want to enforce in moz.build, it > > might make sense to enforce that more globally. Is there a way to do that? > > Or just by reviewing moz.build changes? :) > > Since we're not supporting explicit delete and since this behavior is > dependent on an implementation detail (albeit one with some test coverage), > let's just make it official by implementing __delattr__ and having it always > raise. What should it raise? Good question. AttributeError doesn't seem > right. I'm not sure if there is a built-in exception that is. Maybe just > overload Exception or roll your own child class. I've implemented __delattr__ and raise a MozbuildDeletionError (also in util.py). Is that in line with what you're thinking?

(In reply to Gregory Szorc [:gps] from comment #15) > Comment on attachment 730325 [details] [diff] [review] > Bug 846634 - Part 1: Support EXPORTS[_NAMESPACES] in moz.build > > Review of attachment 730325 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is nearly an r+. I'm still looking at things in util.py. > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +309,5 @@ > > + > > + # Iterate over the list of export filenames, printing out an EXPORTS > > + # declaration for each. > > + for s in strings: > > + backend_file.write('%s += %s\n' % (export_name, s)) > > Possible performance concern: writing out separate variable assignments will > surely take longer to parse than a single assignment with space-delimited > filenames. Will will be able to see a difference? I don't know. This file is > read by machines, not humans. So I'm inclined to say not to take the risk > and just print a single line. I ran a few tests - this definitely makes a difference when you get to around 10k - 100k files. Probably a bit out of our range, but it's easy enough to make it a single line. I've just done this for the EXPORTS themselves, and it is still one line per namespace. > > ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py > @@ +152,5 @@ > > in the future. > > """), > > > > + 'EXPORTS': (HierarchicalStringList, HierarchicalStringList(), > > + """List of files to be exported, and in which subdirectories. > > Let's mention "include files" or similar somewhere in the description. Ok. This is used for a few files other than .h files, though (js.msg and nssck.api). Just FYI. > > ::: python/mozbuild/mozbuild/test/frontend/test_sandbox.py > @@ +317,5 @@ > > + > > + with self.assertRaises(SandboxExecutionError) as se: > > + sandbox.exec_source('EXPORTS += "foo.h"', 'foo.py') > > + > > + self.assertEqual(se.exception.exc_type, Exception) > > self.assertIsInstance(se.exception.exc_type, Exception) > > @@ +319,5 @@ > > + sandbox.exec_source('EXPORTS += "foo.h"', 'foo.py') > > + > > + self.assertEqual(se.exception.exc_type, Exception) > > + self.assertEqual(str(se.exception.exc_value), > > + "Expected a list of strings, not <type 'unicode'>") > > Py3 warning: This will read "<type 'str'>" I think this should work for both now? > > @@ +371,5 @@ > > + > > + with self.assertRaises(SandboxExecutionError) as se: > > + sandbox.exec_source(source, 'foo.py') > > + > > + self.assertEqual(se.exception.exc_type, AttributeError) > > So, all these tests are really just basic tests for HierarchicalStringList > and aren't EXPORTS specific. It isn't really sandbox specific either. Could > you add the basic HierarchicalStringList tests to test_utils.py? > > Also, where are all the tests showing that proper assignments work? I guess > we have most of it in test_emitter.py, but it would be really nice to > consolidate the tests for HierarchicalStringList as a basic type. Yeah, makes sense. I've almost all tests over to test_utils.py, and added some for proper assignments. The only one still in test_sandbox.py is the one where we try to say EXPORTS = "foo.h", since the error comes from the sandbox, not from HierarchicalStringList. > > ::: python/mozbuild/mozbuild/util.py > @@ +15,5 @@ > > > > from StringIO import StringIO > > > > +if sys.version_info[0] == 3: > > + from configparser import RawConfigParser > > Why do you import RawConfigParser? Oops, maybe I should actually look at the code I'm blindly copying :)

Comment on attachment 733341 [details] [diff] [review] Bug 846634 - Part 1: Support EXPORTS[_NAMESPACES] in moz.build Review of attachment 733341 [details] [diff] [review]: ----------------------------------------------------------------- I think this is good enough to land. We can always refine later as cracks are discovered. ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +312,5 @@ > + if strings: > + backend_file.write('%s +=' % export_name) > + for s in strings: > + backend_file.write(' %s' % s) > + backend_file.write('\n') I'd just do all the string foo in Python instead of involving the I/O layer: backend_file.write('%s += %s\n' % (export_name, ' '.join(strings)) ::: python/mozbuild/mozbuild/util.py @@ +256,5 @@ > + > + exports = self._get_exportvariable(name) > + if not isinstance(value, HierarchicalStringList): > + exports._check_list(value) > + exports._strings = value If the value is a HierarchicalStringList, what happens? I think the write just gets ignored. That seems wrong. @@ +276,5 @@ > + return self._children.setdefault(name, HierarchicalStringList()) > + > + def _check_list(self, value): > + if not isinstance(value, list): > + raise ValueError('Expected a list of strings, not %s' % type(value)) Strictly speaking we should probably allow other iterable types. But meh. I'm fine with explicitly restricting this to list instances.

(In reply to Gregory Szorc [:gps] from comment #21) > Comment on attachment 733341 [details] [diff] [review] > Bug 846634 - Part 1: Support EXPORTS[_NAMESPACES] in moz.build > > Review of attachment 733341 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this is good enough to land. We can always refine later as cracks > are discovered. > > ::: python/mozbuild/mozbuild/backend/recursivemake.py > @@ +312,5 @@ > > + if strings: > > + backend_file.write('%s +=' % export_name) > > + for s in strings: > > + backend_file.write(' %s' % s) > > + backend_file.write('\n') > > I'd just do all the string foo in Python instead of involving the I/O layer: > > backend_file.write('%s += %s\n' % (export_name, ' '.join(strings)) Yeah, good point. Done. > > ::: python/mozbuild/mozbuild/util.py > @@ +256,5 @@ > > + > > + exports = self._get_exportvariable(name) > > + if not isinstance(value, HierarchicalStringList): > > + exports._check_list(value) > > + exports._strings = value > > If the value is a HierarchicalStringList, what happens? I think the write > just gets ignored. That seems wrong. I admit to not fully understanding the __setattr__ part, but from what I can gather by adding print statements, we always get a __setattr__ on the parent object with the HierarchicalStringList of the child. So EXPORTS.foo += ['bar.h'] will get a __setattr__('foo', <HierarchicalStringList for foo>) called on the HierarchicalStringList for EXPORTS. Since we aren't actually setting the attribute, it gets ignored. I've added a comment that hopefully explains this correctly.

try results: https://tbpl.mozilla.org/?tree=Try&rev=3cbbe855bcdb

We need this whiteboard content so this bug isn't automatically closed when this part merges to central. Also, since mshal and joey have module peer in the special conversion sub-module, you should be able to finish this bug (or get close to it) on your own.

Most of the changes in Part 2 were automatically done, but the following are probably worth highlighting: accessible/public/ia2 gfx/cairo/cairo/src js/src And all conditionals are moved over manually.

Comment on attachment 736003 [details] [diff] [review] Bug 846634 - Part 2: Move EXPORTS to moz.build Some of the file lists assigned have been sorted. Would hope this is not the case but might processing in a different order expose dependency problems in the conversion that are not currently visibile { if so yes they need to be fixed but porting should roughly be a nop }. ===> accessible/public/moz.build b/accessible/public/moz.build +EXPORTS += [ + 'nsIAccessibilityService.h', +] Cosmetic change but assignment using a single value could be made on one line. ===> accessible/public/ia2/moz.build +EXPORTS += [x.replace('.idl', '.h') for x in midl_enums] +EXPORTS += [x.replace('.idl', '.h') for x in midl_interfaces] +EXPORTS += [x.replace('.idl', '_i.c') for x in midl_interfaces] A regexpr(.idl$) or rsplit() could be used here to avoid stray replacement on multiple string matches. ===> accessible/src/xpcom/moz.build EXPORTS := xpcAccEvents.h EXPORTS += 'xpcAccEvents.h' ] Has conversion of ':=' to += been checked to verify the file list was empty prior to assigning xpcAccEvents.h ? If not we may begin processing more files than would normally be handled. += is the correct flavor overall to handle the other problem of truncating a valid list of files but this change might pose a change in behavior for the conversion. Stopping at: browser/comonents/about/Makefile.in

Comment on attachment 736003 [details] [diff] [review] Bug 846634 - Part 2: Move EXPORTS to moz.build --- a/content/html/content/src/moz.build +if CONFIG['MOZ_MEDIA']: + EXPORTS.mozilla.dom += [ + 'HTMLSourceElement.h', + 'MediaError.h', + ] This MOZ_MEDIA conditional was not present in the original. Probably should submit a makefile edit first then port to keep the two in sync.

(In reply to jarmstrong from comment #31) > Comment on attachment 736003 [details] [diff] [review] > Bug 846634 - Part 2: Move EXPORTS to moz.build > > --- a/content/html/content/src/moz.build > > +if CONFIG['MOZ_MEDIA']: > + EXPORTS.mozilla.dom += [ > + 'HTMLSourceElement.h', > + 'MediaError.h', > + ] > > This MOZ_MEDIA conditional was not present in the original. Probably should > submit a makefile edit first then port to keep the two in sync. That's because MOZ_MEDIA was removed just now in bug 857022; just remove the conditional.

Comment on attachment 736003 [details] [diff] [review] Bug 846634 - Part 2: Move EXPORTS to moz.build dom/src/notification/{Makefile.in,moz.build} - wip(?) - makefile contains export variables. - moz.build EXPORT.mozilla.dom does not include DesktopNotification.h

(In reply to jarmstrong from comment #30) > Comment on attachment 736003 [details] [diff] [review] > Bug 846634 - Part 2: Move EXPORTS to moz.build > > Some of the file lists assigned have been sorted. Would hope this is not > the case but might processing in a different order expose dependency > problems in the conversion that are not currently visibile { if so yes they > need to be fixed but porting should roughly be a nop }. It looks like the sorting comes from mozbuild-migrate. I don't believe there are any problems with this (at least for EXPORTS). > > > ===> accessible/public/moz.build b/accessible/public/moz.build > > +EXPORTS += [ > + 'nsIAccessibilityService.h', > +] Also from using mozbuild-migrate. Some others that I've done (eg: XPIDL_SOURCES) are also like this. I don't mind going back and changing them, but we should probably do it the same for all variables. > > Cosmetic change but assignment using a single value could be made on one > line. > > > ===> accessible/public/ia2/moz.build > > +EXPORTS += [x.replace('.idl', '.h') for x in midl_enums] > +EXPORTS += [x.replace('.idl', '.h') for x in midl_interfaces] > +EXPORTS += [x.replace('.idl', '_i.c') for x in midl_interfaces] > > A regexpr(.idl$) or rsplit() could be used here to avoid stray replacement > on multiple string matches. Ahh, good point. I first tried: EXPORTS += [re.sub('.idl$', '.h', x) for x in midl_enums] EXPORTS += [re.sub('.idl$', '.h', x) for x in midl_interfaces] EXPORTS += [re.sub('.idl$', '_i.c', x) for x in midl_interfaces] But for some reason this fails in Windows (I guess on other platforms, 're' is imported automatically, whereas on Windows it is not?). With rsplit, it looks like this: EXPORTS += [x.rsplit('.idl', 1)[0] + '.h' for x in midl_enums] EXPORTS += [x.rsplit('.idl', 1)[0] + '.h' for x in midl_interfaces] EXPORTS += [x.rsplit('.idl', 1)[0] + '_i.c' for x in midl_interfaces] But that seems somewhat ugly. Any advice on how best to proceed? (Or other suggestions to try?) > > > ===> accessible/src/xpcom/moz.build > > EXPORTS := xpcAccEvents.h > EXPORTS += 'xpcAccEvents.h' ] > > Has conversion of ':=' to += been checked to verify the file list was empty > prior to assigning xpcAccEvents.h ? If not we may begin processing more > files than would normally be handled. I've just checked now - I did a 'make export' before and after this patch, and the list of header files under dist/include is the same and both cases. So we aren't processing new files that weren't being included before.

(In reply to :Ms2ger from comment #32) > (In reply to jarmstrong from comment #31) > > Comment on attachment 736003 [details] [diff] [review] > > Bug 846634 - Part 2: Move EXPORTS to moz.build > > > > --- a/content/html/content/src/moz.build > > > > +if CONFIG['MOZ_MEDIA']: > > + EXPORTS.mozilla.dom += [ > > + 'HTMLSourceElement.h', > > + 'MediaError.h', > > + ] > > > > This MOZ_MEDIA conditional was not present in the original. Probably should > > submit a makefile edit first then port to keep the two in sync. > > That's because MOZ_MEDIA was removed just now in bug 857022; just remove the > conditional. Yeah, looks like I goofed that one up when I rebased before posting the patch. Good catch!

(In reply to Joey Armstrong [:joey] from comment #33) > Comment on attachment 736003 [details] [diff] [review] > Bug 846634 - Part 2: Move EXPORTS to moz.build > > dom/src/notification/{Makefile.in,moz.build} - wip(?) > - makefile contains export variables. > - moz.build EXPORT.mozilla.dom does not include DesktopNotification.h Looks like another recent change. I'll rebase to include this.

Comment on attachment 736003 [details] [diff] [review] Bug 846634 - Part 2: Move EXPORTS to moz.build Last batch, I can review the patch again when fixes are in. ** unexpanded macro was inserted gfx/graphite2/src/moz.build EXPORTS.graphite2 += [ '$(_PUBLIC_HEADERS)', ] memory/mozalloc/moz.build: build_msvc_wrappers can be used when undefined

> It looks like the sorting comes from mozbuild-migrate. I don't believe there > are any problems with this (at least for EXPORTS). The ordering doesn't matter, so you might as well leave them sorted for the aesthetic value. > > +EXPORTS += [ > > + 'nsIAccessibilityService.h', > > +] > > Also from using mozbuild-migrate. Some others that I've done (eg: > XPIDL_SOURCES) are also like this. I don't mind going back and changing > them, but we should probably do it the same for all variables. I think we ought to leave these as preferred style, since it allows for adding new entries to the list without reformatting. (In fact, if we were to write a moz.build style guide I would probably recommend this as the way to write lists.)

I could use some suggestions for how to handle these cases: 1) accessible/public/ia2 (see https://bugzilla.mozilla.org/show_bug.cgi?id=846634#c34 ) - Do we want to try to preserve defining a local variable like MIDL_INTERFACES and doing extension-replacements in moz.build? If so what is the best way to do that in moz.build? - Or do we want to just explicitly list the files in moz.build? (eg: EXPORTS += ['Accessible2.h', 'Accessible2_i.c'], etc) 2) gfx/graphite2/src - joey found that I accidentally carried over $(_PUBLIC_HEADERS) from Makefile.in during the automatic conversion. It looks like we use the Makefile.in to convert from that external library's definitions (in files.mk) to ones that our rules.mk can use. Do we want to still try to pull definitions from files.mk with moz.build? (If so, how best to do that?) Or do we want to use our own definition in moz.build, possibly by adding an automatic conversion to gfx/graphite2/moz-gr-update.sh ?

(In reply to Michael Shal [:mshal] from comment #39) > I could use some suggestions for how to handle these cases: > > 1) accessible/public/ia2 (see > https://bugzilla.mozilla.org/show_bug.cgi?id=846634#c34 ) > - Do we want to try to preserve defining a local variable like > MIDL_INTERFACES and doing extension-replacements in moz.build? If so what is > the best way to do that in moz.build? > - Or do we want to just explicitly list the files in moz.build? (eg: > EXPORTS += ['Accessible2.h', 'Accessible2_i.c'], etc) If the resulting build rules (via moz.build variables/function calls) are derived from the same common set, my vote is to: a) define the common set and then perform iteration, etc to populate the necessary moz.build files b) If said pattern is common (used in more than a few places), we should consider adding functionality to the moz.build sandbox to cover it. Here, I think we go with "a". Define a local variable then assign to moz.build variables by iterating, using a list comprehension, etc. > 2) gfx/graphite2/src > - joey found that I accidentally carried over $(_PUBLIC_HEADERS) from > Makefile.in during the automatic conversion. It looks like we use the > Makefile.in to convert from that external library's definitions (in > files.mk) to ones that our rules.mk can use. Do we want to still try to pull > definitions from files.mk with moz.build? (If so, how best to do that?) Or > do we want to use our own definition in moz.build, possibly by adding an > automatic conversion to gfx/graphite2/moz-gr-update.sh ? Ugh. Ideally we'll pull in whatever is defined upstream instead of having to hand-maintain moz.build variations. So, I think that means adjusting moz-gr-update.sh. (May involve converting to Python and using the pymake API to "parse" the upstream make files into a .mozbuild file.) If we really want to land this bug, we could always do it by hand now, drop a note in the conversion script that changes to files.mk will need to be manually ported to a .mozbuild file, then follow-up with fixing moz-gr-update.sh later. I guess that answer partially depends on how often Graphite is updated and how quickly we'd fix moz-gr-update.sh. Since files.mk has exactly 3 changes in 1.3 years, I'm not too concerned. But we should still ask...

This has the following changes over the previous patch: accessible/public/ia2 - just made midl_interfaces not have an idl extension, so creating the list of files is easier, and works without importing anything content/html/content/src - removed the incorrect MOZ_MEDIA condition block gfx/graphite2 - added a note to moz-gr-update.sh, and manually copied EXPORTS from files.mk (the upstream manifest) to moz.build memory/mozalloc - fixed build_msvc_wrappers issue rebased, which affected the following directories: content/svg/content/src dom/mobilemessage/src dom/src/notification dom/src/storage gfx/layers xpcom/reflect/xptinfo/public

Comment on attachment 738066 [details] [diff] [review] Bug 846634 - Part 2: Move EXPORTS to moz.build Good workaround for the midl_ filename generation.

(In reply to Joey Armstrong [:joey] from comment #42) > Comment on attachment 738066 [details] [diff] [review] > Bug 846634 - Part 2: Move EXPORTS to moz.build > > Good workaround for the midl_ filename generation. Please add pointers to accessible/public/ia2/{Makefile.in,moz.build} pointing to the other definition of that list.

Add comments per :Ms2ger.

try results: https://tbpl.mozilla.org/?tree=Try&rev=7d28ea77a730

Try looks good. This looks clear to land. Please ping dev-platform so people know what's up. We'll need to update rules.mk to disallow EXPORTS and EXPORT_NAMESPACES once comm-central is on board.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/51644264df6d

Comment on attachment 739211 [details] [diff] [review] Stragglers Review of attachment 739211 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - thanks for catching these!