Closed Bug 1252931 Opened 4 years ago Closed 3 years ago

Move GDB autoload files to moz.build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: cmanchester, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

There are only a few of these, but they look like an easy conversion to FINAL_TARGET_{PP}_FILES

https://dxr.mozilla.org/mozilla-central/search?q=path%3AMakefile.in+AUTOLOAD&redirect=false&case=true
The -Dtopsrcdir=$(abspath $(srcdir)/..) part seems problematic.
moz.build files expose TOPSRCDIR, if that's not already absolute we could make it so and you could set it in DEFINES:
http://gecko.readthedocs.org/en/latest/build/buildsystem/mozbuild-symbols.html#topsrcdir
Assignee: nobody → mshal
I don't believe we have a way in mozbuild to preserve where these files currently end up in the objdir, so this does move some things around a bit:

js-gdb.py was originally in obj/js/src/shell/ and obj/dist/bin/, but now is only in obj/dist/bin/

jsapi-tests-gdb.py was originally in obj/js/src/jsapi-tests/, but now is in obj/dist/bin/

gdb-tests-gdb.py was originally in obj/js/src/gdb/ and obj/dist/bin, but now is only in obj/dist/bin/

r?ted for the actual conversion, r?jimb on whether or not the file moves are ok. If this breaks some workflows, I think we'll have to add something to allow us to preprocess from the srcdir to the corresponding objdir, rather than to a directory based off of FINAL_TARGET.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c998b5ed3d6c
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

https://reviewboard.mozilla.org/r/39523/#review36207

Looks fine to me if jimb signs off on the files only being in dist/bin.
Attachment #8729624 - Flags: review?(ted) → review+
How does this change affect SpiderMonkey-only builds? Here's what I mean by that:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

The pretty-printers must be available without any extra attention from the developer when SpiderMonkey is built stand-alone in the fastest way.
Flags: needinfo?(mshal)
There's also a .gdbinit, .lldbinit, and .ycm_extra_conf.py installed from build/Makefile.in:
https://hg.mozilla.org/mozilla-central/file/31481f9ebdda792e0ec01d8a007de8b193bc206f/build/Makefile.in#l15

They all get installed to the root of the objdir.
Fwiw, I asked in #jsapi how people ran the shell, and the small sample I got was 100% obj/js/src/js, where the autoload file is not currently installed.
(In reply to :Ms2ger from comment #8)
> Fwiw, I asked in #jsapi how people ran the shell, and the small sample I got
> was 100% obj/js/src/js, where the autoload file is not currently installed.

That's a symlink to obj/js/src/shell/js, and there is a GDB autoload file installed as obj/js/src/shell/js-gdb.py.

If I understand correctly, that means that this patch will stop installing the autoload file in the location where most JS developers currently use it.
Flags: needinfo?(mshal)
Attachment #8729624 - Flags: review?(jimb)
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

https://reviewboard.mozilla.org/r/39523/#review36391

Since this patch causes the autoload file not to be installed in the location where most SpiderMonkey developers use it, I don't think it's a good change.

For SpiderMonkey-only builds, GDB autoload files are needed for all three executables we build:

js/src/shell/js-gdb.py
js/src/jsapi-tests/jsapi-tests-gdb.py
js/src/gdb/gdb-tests-gdb.py
Attachment #8729624 - Flags: review-
Any suggestions on how we support this in mozbuild? It sounds like we have a couple of odd-ball cases for the remaining PP_TARGETS and INSTALL_TARGETS:

1) In this bug, we need to preprocess a file and put the output in both obj/$(relativesrcdir) and obj/dist/bin.

2) As ted points out in #c7, there are a few cases where things get installed to the root of the objdir, or objdir/_valgrind

3) toolkit/mozapps/update/tests/Makefile.in installs preprocessed files to _tests, and I don't think we can change FINAL_TARGET here.

4) js also has a couple random rules for things like copying the js shell into obj/js/src/: https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/js/src/shell/Makefile.in#24

There might be others as well, but I only conducted a brief review of the remaining INSTALL_TARGETS & PP_TARGETS.

Should we have hierarchical variables like OBJDIR_PP_FILES and OBJDIR_FILES to handle this? Or should we try to add targeted variables for each of these instances? Something like ROOT_OBJDIR_FILES and ROOT_OBJDIR_PP_FILES for the things in build/Makefile.in, VALGRIND_FILES for objdir/_valgrind, JS_SRC_FILES for objdir/js/src, etc.

Other suggestions welcome! I'm happy to implement it whichever seems most appropriate.
Flags: needinfo?(gps)
It might make sense to somehow tie the GDB autoload scripts to rules that generate executables. If we could say, in moz.build, "This executable has a GDB autoload script whose template is here", then the same code that places the executable where it lands could take care of the autoload, too. After all, they are a pair.
(In reply to Jim Blandy :jimb from comment #9)
> (In reply to :Ms2ger from comment #8)
> > Fwiw, I asked in #jsapi how people ran the shell, and the small sample I got
> > was 100% obj/js/src/js, where the autoload file is not currently installed.
> 
> That's a symlink to obj/js/src/shell/js, and there is a GDB autoload file
> installed as obj/js/src/shell/js-gdb.py.

This is all really unfortunate. I would like developers to treat the objdir as a black box because every time they don't, it makes it harder for us build system maintainers to change things. One of the little things `mach run` does is abstract where binaries are and how to run them so we can do things like change where binaries are installed to.

That being said, the least opaque part of the objdir is the dist/ directory because content therein tends to resemble things we package either verbatim or with light massaging. So to developers running binaries from obj/js/src, I tell you that you should a) use `mach` (if available - which may not be for SM devs) b) use obj/dist/bin.

Speaking of mach, there was talk ~1 year ago of writing a special mach driver for SM devs. I'm not sure who I was talking to about that. But this bug serves as a reminder that it would be nice to have so SM devs aren't peeking inside our objdir black box :)
(In reply to Michael Shal [:mshal] from comment #11)
> Should we have hierarchical variables like OBJDIR_PP_FILES and OBJDIR_FILES
> to handle this?

I like this because it is simple. We can add targeted variables if any class of file becomes too common or we gain benefits from differentiating from the file type. At this juncture, the important thing is moving things to moz.build. We can repaint the bikeshed later.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #13)
> That being said, the least opaque part of the objdir is the dist/ directory
> because content therein tends to resemble things we package either verbatim
> or with light massaging. So to developers running binaries from obj/js/src,
> I tell you that you should a) use `mach` (if available - which may not be
> for SM devs) b) use obj/dist/bin.

Okay, well, I tell *you* that mach doesn't offer us anything comparable to the JS-only builds you find so "unfortunate". I'm pretty sure the SpiderMonkey folks would be happy to use mach if it actually met our needs.
That's bug 1210848; let's keep this bug focused on the autoload files, please.
(In reply to Jim Blandy :jimb from comment #12)
> It might make sense to somehow tie the GDB autoload scripts to rules that
> generate executables. If we could say, in moz.build, "This executable has a
> GDB autoload script whose template is here", then the same code that places
> the executable where it lands could take care of the autoload, too. After
> all, they are a pair.

This sounds like a good idea to me, but I suspect that since we'll need something slightly generic to handle the other install rules mentioned (ie: installing the js shell into objdir/js/src), we'll have something that will cover getting the autoload scripts into the right place without explicitly pairing them in moz.build. I'll try to take a look though to see if it would be easy to do.
I wonder if (for standalone JS builds) we could just set FINAL_TARGET up one level so the js shell binary and the gdb file wound up in the same place, which would also get rid of the "copy the js shell up one level" rule?

(Incidentally, I added the "copy the shell binary" rule, because I had to put the rule to build the shell in a subdir because the Mozilla build system couldn't produce a library and a binary from the same directory, and JS devs expected the js binary to be in the place where they ran configure && make.)
(In reply to :Ms2ger from comment #8)
> Fwiw, I asked in #jsapi how people ran the shell, and the small sample I got
> was 100% obj/js/src/js, where the autoload file is not currently installed.

GDB follows the symlink, and checks for the autoload file where the actual executable lives. Note the "If they cause trouble" message at the bottom:

sergei:src$ gdb obj~/js/src/js
GNU gdb (GDB) Fedora 7.9.1-20.fc22
Copyright (C) 2015 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from obj~/js/src/js...done.
Loading JavaScript value pretty-printers; see js/src/gdb/README.
If they cause trouble, type: disable pretty-printer .* SpiderMonkey
(gdb)
(In reply to Michael Shal [:mshal] from comment #17)
> (In reply to Jim Blandy :jimb from comment #12)
> > It might make sense to somehow tie the GDB autoload scripts to rules that
> > generate executables. If we could say, in moz.build, "This executable has a
> > GDB autoload script whose template is here", then the same code that places
> > the executable where it lands could take care of the autoload, too. After
> > all, they are a pair.
> 
> This sounds like a good idea to me, but I suspect that since we'll need
> something slightly generic to handle the other install rules mentioned (ie:
> installing the js shell into objdir/js/src), we'll have something that will
> cover getting the autoload scripts into the right place without explicitly
> pairing them in moz.build. I'll try to take a look though to see if it would
> be easy to do.

Aren't the two questions here independent?

1) Does this executable have a GDB autoload file to go with it?

2) Where should copies of this executable be placed?

It seems like one ought to be able to change either of those without thinking about the other, and have things just work.
(In reply to Jim Blandy :jimb from comment #20)
> Aren't the two questions here independent?
> 
> 1) Does this executable have a GDB autoload file to go with it?
> 
> 2) Where should copies of this executable be placed?
> 
> It seems like one ought to be able to change either of those without
> thinking about the other, and have things just work.

Part 2 is effectively FINAL_TARGET today, which I think is why ted suggested changing that for standlone js builds in #c18. That would change the behavior between standlone js builds and full builds though - in one case we'd have js in obj/src/js, and in the other in obj/dist/bin. At this point I'm not too keen on trying to change the location of things and break workflows.

As for part 1, my concern was if it makes sense to add a specific mozbuild variable for something that is used by three Makefiles (js/src/ shell, jsapi-tests, and gdb). Given the other INSTALL_TARGETS / PP_TARGETS that need to be converted, I think we'll need something a little more flexible in how things can be installed. And if we need that anyway, that can handle installing the autoload files to the right places.

Perhaps that can just be an implementation detail hidden behind the GeckoProgram() template, which could take an autoload=foo or some such argument.
Well, there is patch to add some Gecko GDB pretty-printers in bug 985566. So it's not just SpiderMonkey.

I will leave it to your judgement whether something specific to GDB autoloads is best, or something more generic. But I think the GDB autoload requirement, that it must sit next to the executable, is probably pretty unusual.
Depends on: 1254682
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39523/diff/1-2/
Attachment #8729624 - Attachment description: MozReview Request: Bug 1252931 - Move some AUTOLOAD install targets to moz.build; r?ted, jimb → MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps
Attachment #8729624 - Flags: review- → review?(gps)
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

Hmm, ted didn't r+ the new patches.
Attachment #8729624 - Flags: review+
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

https://reviewboard.mozilla.org/r/39523/#review37991

::: python/mozbuild/mozbuild/frontend/emitter.py:879
(Diff revision 2)
>                  if base == 'defaults/pref':
>                      has_prefs = True
>                  if mozpath.split(base)[0] == 'res':
>                      has_resources = True
>                  for f in files:
> -                    if var == 'FINAL_TARGET_PP_FILES':
> +                    if 'PP_FILES' in var:

This feels dangerous. Let's iterate over specific variable names instead.
Attachment #8729624 - Flags: review?(gps) → review+
Comment on attachment 8733099 [details]
MozReview Request: Bug 1252931 - Remove INSTALL/PP_TARGETS from js/src/*; r=gps

https://reviewboard.mozilla.org/r/41539/#review37993

I'm not too crazy about topsrcdir going in global defines. Can we not only define it for preprocessor invocations for the specific files?

Also, please do a try push (if you haven't already) to ensure standalone SM builds work.
Attachment #8733099 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #27)
> I'm not too crazy about topsrcdir going in global defines. Can we not only
> define it for preprocessor invocations for the specific files?

The recursive make backend supports a _FLAGS argument for PP_TARGETS, but we don't currently have a way of setting that from mozbuild. Followup or do you want that fixed before this lands?
Flags: needinfo?(gps)
Followup.
Flags: needinfo?(gps)
No longer depends on: 1254682
Filed bug 1259530.

Also while trying to use OBJDIR_FILES / OBJDIR_PP_FILES in a few other conversions, I found a few small issues in recursivemake.py:

1) Using both FINAL_TARGET_PP_FILES and OBJDIR_PP_FILES in the same moz.build file conflicted with each other, since the backend was using the same variable names for both. I passed in a 'name' argument to work around this.

2) Similarly, _process_objdir_files() was not using the 'i' counter when writing out the variable names, so trying to have multiple OBJDIR_FILES would fail.

I also fixed the "'PP_FILES' in var" issue. Since the changes are not insignificant, I'm requesting re-review on this patch. The actual INSTALL_TARGET removal is unchanged.
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39523/diff/2-3/
Attachment #8729624 - Flags: review?(ted)
Comment on attachment 8733099 [details]
MozReview Request: Bug 1252931 - Remove INSTALL/PP_TARGETS from js/src/*; r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41539/diff/1-2/
Attachment #8733099 - Attachment description: MozReview Request: Bug 1252931 - Remove INSTALL/PP_TARGETS from js/src/*; r?gps → MozReview Request: Bug 1252931 - Remove INSTALL/PP_TARGETS from js/src/*; r=gps
And I forgot to mention, this was rebased so as to not depend on bug 1254682.
Attachment #8729624 - Flags: review?(ted)
Attachment #8729624 - Flags: review?(gps)
Attachment #8729624 - Flags: review+
Blocks: 1259555
Blocks: 1259554
Comment on attachment 8729624 [details]
MozReview Request: Bug 1252931 - Add support for generic OBJDIR_FILES and OBJDIR_PP_FILES; r?gps

https://reviewboard.mozilla.org/r/39523/#review38945

LGTM
Attachment #8729624 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/b5d975e3f982
https://hg.mozilla.org/mozilla-central/rev/491142a0995b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.