Closed
Bug 1459721
Opened 6 years ago
Closed 6 years ago
remove the dist_idl install manifest
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
Attachments
(8 files, 1 obsolete file)
1.74 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
6.79 KB,
patch
|
Details | Diff | Splinter Review | |
8.37 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
6.57 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
4.53 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
9.81 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
We should just stop copying all the IDL files to the objdir everytime we build. It makes generating IDL files at build time difficult, and there's no reason to do it anytime, since we e.g. no longer care about the SDK.
Assignee | ||
Comment 1•6 years ago
|
||
This method is only called in one place, and it doesn't pass allow_existing. Whatever ugly thing this keyword was working around doesn't exist anymore, so let's get rid of it. This is just a cleanup.
Attachment #8973810 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 2•6 years ago
|
||
This member is unused, so we might as well dispense with it. Another cleanup before we get to the intersting stuff.
Attachment #8973811 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 3•6 years ago
|
||
The current IDL build setup assumes that all IDL files can be found in a single directory. This setup requires that all IDL files be copied to a single directory, which is suboptimal in terms of disk I/O and also complicates things like generating IDL files at build time. As a first step in moving away from this state of affairs, xpidl-process.py needs to be taught that the input IDL files could potentially be found in multiple directories. The current setup can just specify $(DIST)/idl as the lone directory to examine. Future patches will change this to examine multiple directories.
Attachment #8973812 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 4•6 years ago
|
||
Building on the last patch, we can change the build process to pass in the directories where the input IDL files can be found. It is convenient to pass in just the relative source directory paths, to encourage people to not look in the object directory and to make the command lines slightly shorter. xpidl-process.py still assumes that included IDL files can be found by looking in a single directory. We add a single -I argument to the invocation of xpidl-process.py to accommodate this short-sightedness.
Attachment #8973813 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 5•6 years ago
|
||
The previous patch required us to pass a single -I argument pointing at $(DIST)/idl so IDL include statements would work correctly. This patch lifts that limitation and explicitly points xpidl-process.py at the locations of all the IDL source directories to search for included IDL files. Invocations of xpidl-process.py no longer depend on IDL files being copied to the objdir.
Attachment #8973814 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 6•6 years ago
|
||
The tail end of the xpidl Makefile.in contains a line, generated for every xpt file: $(1): $(addsuffix .idl,$(addprefix $(dist_idl_dir)/,$($(basename $(notdir $(1)))_deps))) This line, in context, is saying that the xpt file depends on all of its input IDL files. But xpidl-process.py already generates this information when we pass it --depsdir, which we do. So this code is redundant with what we already generate, and it can be removed.
Attachment #8973815 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Comment 7•6 years ago
|
||
We no longer need to install the IDL files to the objdir for processing. \o/
Attachment #8973816 -
Flags: review?(core-build-config-reviews)
Updated•6 years ago
|
Attachment #8973810 -
Flags: review?(core-build-config-reviews) → review+
Comment 8•6 years ago
|
||
Comment on attachment 8973811 [details] [diff] [review] part 2 - remove install_target member from XPIDLManager.modules Review of attachment 8973811 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/common.py @@ +79,5 @@ > raise Exception('IDL already registered: %s' % entry['basename']) > > self.idls[entry['basename']] = entry > + # First element is a set of interface file basenames (no extension). > + t = self.modules.setdefault(entry['module'], (set(),)) This one-element tuple doesn't seem to be buying us anything, can't you just use the set as the value?
Attachment #8973811 -
Flags: review?(core-build-config-reviews) → review+
Comment 9•6 years ago
|
||
Comment on attachment 8973812 [details] [diff] [review] part 3 - enable multiple input paths for xpidl-process.py Review of attachment 8973812 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/action/xpidl-process.py @@ +44,5 @@ > rule.add_dependencies(iter_modules_in_path(topsrcdir)) > > + def read_stem(stem): > + idl = '%s.idl' % stem > + for p in input_dirs: So we search every directory for each file in case it has the one we care about? This seems clumsy and potentially slow, shouldn't we just pass the full paths this script and let it derive the set of directories from that if necessary? Does this get better in a patch later in the series?
Attachment #8973812 -
Flags: review?(core-build-config-reviews)
Comment 10•6 years ago
|
||
Comment on attachment 8973813 [details] [diff] [review] part 4 - explicitly specify input directories for xpidl modules Review of attachment 8973813 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/recursivemake.py @@ +1066,5 @@ > > mk = Makefile() > > for module in xpt_modules: > + (sources, directories) = modules[module] Parens not necessary. @@ +1082,5 @@ > # reference to the new .idl. Since the new .idl presumably has > # an mtime newer than the .xpt, it will trigger xpt generation. > mk.add_statement('%s_deps = %s' % (module, ' '.join(deps))) > > + mk.add_statement('%s_dirs = %s' % (module, ' '.join(directories))) A separate variable here doesn't really seem necessary, but ok.
Attachment #8973813 -
Flags: review?(core-build-config-reviews) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8973814 [details] [diff] [review] part 5 - explicitly specify include directories for xpidl files Review of attachment 8973814 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/tup.py @@ +680,5 @@ > ] > > for d in directories: > cmd.extend(['--input-dir', d]) > + for d in all_idl_directories: This kinda looks like it's going to iterate of the files instead of the directories, does this pass the tup build on try?
Attachment #8973814 -
Flags: review?(core-build-config-reviews) → review+
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #8) > ::: python/mozbuild/mozbuild/backend/common.py > @@ +79,5 @@ > > raise Exception('IDL already registered: %s' % entry['basename']) > > > > self.idls[entry['basename']] = entry > > + # First element is a set of interface file basenames (no extension). > > + t = self.modules.setdefault(entry['module'], (set(),)) > > This one-element tuple doesn't seem to be buying us anything, can't you just > use the set as the value? We will be adding more elements later in the series, seemed a bit silly to switch it back and forth. (In reply to Chris Manchester (:chmanchester) from comment #9) > ::: python/mozbuild/mozbuild/action/xpidl-process.py > @@ +44,5 @@ > > rule.add_dependencies(iter_modules_in_path(topsrcdir)) > > > > + def read_stem(stem): > > + idl = '%s.idl' % stem > > + for p in input_dirs: > > So we search every directory for each file in case it has the one we care > about? This seems clumsy and potentially slow, shouldn't we just pass the > full paths this script and let it derive the set of directories from that if > necessary? Does this get better in a patch later in the series? It doesn't get better later in the series, but I am happy to add an additional patch to clean that up. xpidl-process.py expects the IDL files to be passed in as stems, and it seemed easier to let that be. I can clean it up in a part 8 or so, would that be OK?
Flags: needinfo?(cmanchester)
Updated•6 years ago
|
Attachment #8973815 -
Flags: review?(core-build-config-reviews) → review+
Updated•6 years ago
|
Attachment #8973816 -
Flags: review?(core-build-config-reviews) → review+
Comment 13•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #12) > (In reply to Chris Manchester (:chmanchester) from comment #8) > > ::: python/mozbuild/mozbuild/backend/common.py > > @@ +79,5 @@ > > > raise Exception('IDL already registered: %s' % entry['basename']) > > > > > > self.idls[entry['basename']] = entry > > > + # First element is a set of interface file basenames (no extension). > > > + t = self.modules.setdefault(entry['module'], (set(),)) > > > > This one-element tuple doesn't seem to be buying us anything, can't you just > > use the set as the value? > > We will be adding more elements later in the series, seemed a bit silly to > switch it back and forth. > > (In reply to Chris Manchester (:chmanchester) from comment #9) > > ::: python/mozbuild/mozbuild/action/xpidl-process.py > > @@ +44,5 @@ > > > rule.add_dependencies(iter_modules_in_path(topsrcdir)) > > > > > > + def read_stem(stem): > > > + idl = '%s.idl' % stem > > > + for p in input_dirs: > > > > So we search every directory for each file in case it has the one we care > > about? This seems clumsy and potentially slow, shouldn't we just pass the > > full paths this script and let it derive the set of directories from that if > > necessary? Does this get better in a patch later in the series? > > It doesn't get better later in the series, but I am happy to add an > additional patch to clean that up. xpidl-process.py expects the IDL files > to be passed in as stems, and it seemed easier to let that be. I can clean > it up in a part 8 or so, would that be OK? Sure, although I think simplifying here would mean we wouldn't need the extra variable I mentioned in part 4 either.
Flags: needinfo?(cmanchester)
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Chris Manchester (:chmanchester) from comment #13) > Sure, although I think simplifying here would mean we wouldn't need the > extra variable I mentioned in part 4 either. Oh! I think we can remove that makefile variable regardless; I think I had it in there before I realized I could just consult the individual ${module}_dirs values with a `foreach`.
Assignee | ||
Comment 15•6 years ago
|
||
The build system knows at build-backend time where to find each IDL file; making xpidl-process.py rediscover this by requiring xpidl-process.py to search through directories to find input IDL files is silly. To rememdy this, we're going to modify things so full paths are passed into the script. Those paths can then be used directly, with no searching.
Attachment #8975072 -
Flags: review?(cmanchester)
Assignee | ||
Comment 16•6 years ago
|
||
Here, let's provide a patch that doesn't break the tup backend.
Attachment #8975106 -
Flags: review?(cmanchester)
Assignee | ||
Updated•6 years ago
|
Attachment #8975072 -
Attachment is obsolete: true
Attachment #8975072 -
Flags: review?(cmanchester)
Comment 17•6 years ago
|
||
Comment on attachment 8975106 [details] [diff] [review] part 8 - pass full paths for IDL files to xpidl-process.py Review of attachment 8975106 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating this. ::: python/mozbuild/mozbuild/backend/tup.py @@ +813,5 @@ > > all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module)) > outputs = ['%s.xpt' % module] > + stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0], > + map(mozpath.basename, idls)))) I'm told that list comprehensions or generator expressions are more idiomatic in python than `map`... the nested `map` doesn't really seem necessary. The cast to a list doesn't, either. How about: `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`?
Attachment #8975106 -
Flags: review?(cmanchester) → review+
Assignee | ||
Comment 18•6 years ago
|
||
Thanks for the review. (In reply to Chris Manchester (:chmanchester) from comment #17) > ::: python/mozbuild/mozbuild/backend/tup.py > @@ +813,5 @@ > > > > all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module)) > > outputs = ['%s.xpt' % module] > > + stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0], > > + map(mozpath.basename, idls)))) > > I'm told that list comprehensions or generator expressions are more > idiomatic in python than `map`... the nested `map` doesn't really seem > necessary. The cast to a list doesn't, either. How about: > `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`? That sounds great; for some reason I thought `sorted` returned an iterator, rather than a list. Just to be clear, with this bit, part 3 has your r+ (cf comment 13), or would you prefer that I fold this part into part 3?
Flags: needinfo?(cmanchester)
Comment 19•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #18) > Thanks for the review. > > (In reply to Chris Manchester (:chmanchester) from comment #17) > > ::: python/mozbuild/mozbuild/backend/tup.py > > @@ +813,5 @@ > > > > > > all_xpts.append('$(MOZ_OBJ_ROOT)/%s/%s.xpt' % (backend_file.relobjdir, module)) > > > outputs = ['%s.xpt' % module] > > > + stems = list(sorted(map(lambda idl: mozpath.splitext(idl)[0], > > > + map(mozpath.basename, idls)))) > > > > I'm told that list comprehensions or generator expressions are more > > idiomatic in python than `map`... the nested `map` doesn't really seem > > necessary. The cast to a list doesn't, either. How about: > > `sorted(mozpath.splitext(mozpath.basename(f))[0] for f in idls)`? > > That sounds great; for some reason I thought `sorted` returned an iterator, > rather than a list. > > Just to be clear, with this bit, part 3 has your r+ (cf comment 13), or > would you prefer that I fold this part into part 3? Yes, this has r+, it's fine as a separate patch if that simplifies anything.
Flags: needinfo?(cmanchester)
Comment 20•6 years ago
|
||
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0bd15e1d5d part 1 - remove allow_existing keyword arg from register_idl; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/94988ff9a3f3 part 2 - remove install_target member from XPIDLManager.modules; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/55957988163d part 3 - enable multiple input paths for xpidl-process.py; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/356ba3cc76e1 part 4 - explicitly specify input directories for xpidl modules; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/545f0976650c part 5 - explicitly specify include directories for xpidl files; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/05168b1b6cf7 part 6 - remove redundant dependency code from xpidl Makefile.in; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/7f618d13bd1c part 7 - remove dist_idl install manifest; r=chmanchester https://hg.mozilla.org/integration/mozilla-inbound/rev/d52f0d61dd57 part 8 - pass full paths for IDL files to xpidl-process.py; r=chmanchester
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ed0bd15e1d5d https://hg.mozilla.org/mozilla-central/rev/94988ff9a3f3 https://hg.mozilla.org/mozilla-central/rev/55957988163d https://hg.mozilla.org/mozilla-central/rev/356ba3cc76e1 https://hg.mozilla.org/mozilla-central/rev/545f0976650c https://hg.mozilla.org/mozilla-central/rev/05168b1b6cf7 https://hg.mozilla.org/mozilla-central/rev/7f618d13bd1c https://hg.mozilla.org/mozilla-central/rev/d52f0d61dd57
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•