Closed Bug 1188571 Opened 5 years ago Closed 4 years ago

build/unix/mozconfig.gtk is more than just config

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1242641

People

(Reporter: dustin, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

build/unix/mozconfig.gtk runs some binaries to get configuration:

----
  # pango expects absolute paths in pango.modules, and topsrcdir may vary...
  LD_LIBRARY_PATH=$topsrcdir/gtk3/usr/local/lib \
  PANGO_SYSCONFDIR=$topsrcdir/gtk3/usr/local/etc \
  PANGO_LIBDIR=$topsrcdir/gtk3/usr/local/lib \
  $topsrcdir/gtk3/usr/local/bin/pango-querymodules > $topsrcdir/gtk3/usr/local/etc/pango/pango.modules

  # same with gdb-pixbuf and loaders.cache
  LD_LIBRARY_PATH=$topsrcdir/gtk3/usr/local/lib \
  GDK_PIXBUF_MODULE_FILE=$topsrcdir/gtk3/usr/local/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache \
  GDK_PIXBUF_MODULEDIR=$topsrcdir/gtk3/usr/local/lib/gdk-pixbuf-2.0/2.10.0/loaders \
  $topsrcdir/gtk3/usr/local/bin/gdk-pixbuf-query-loaders > $topsrcdir/gtk3/usr/local/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache

  # mock build environment doesn't have fonts in /usr/share/fonts, but
  # has some in /usr/share/X11/fonts. Add this directory to the
  # fontconfig configuration without changing the gtk3 tooltool package.
  cat << EOF > $topsrcdir/gtk3/usr/local/etc/fonts/local.conf
<?xml version="1.0"?>
<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
  <dir>/usr/share/X11/fonts</dir>
</fontconfig>
EOF
---

This seems to go beyond the "config" notion of a mozconfig: it is running binaries to draw in information from the live system, and it has side-effects (admitted, idempotent side effects).

In a concrete sense, this has caused the error in comment 1 of bug 1188551.  The root here is that PKG_CONFIG_LIBDIR is set incorrectly for this build, but the error messages I got were not helpful -- first an assertion from deep in mach, and then a cryptic linker error output from the mozconfig run.  I humbly submit that such errors should occur at ./configure time, not during the mozconfigs.
Configure is not the right place for these things because they are automation-only things to do. We don't pull tooltool packages from configure ;)

Now, what those things to is set up files that can't be shipped in the tooltool packages because they depend on the parent path. (except the last thing, I just didn't want to update the tooltool package).

We kind of have setup.sh that tooltool runs after unpacking, but we've moved away from this because of the maintenance burden, especially when multiple packages need a setup, because then you have to maintain several such files, with different snippets, and updating becomes a giant PITA.

How about adding an entry to tooltool manifests saying something like setup: "setup.sh", in which case tooltool would execute the setup.sh script *in the tooltool package*?
Flags: needinfo?(mshal)
There may be a need for a specific tooltool package to e.g run registration
commands of some sort. There used to be a global way to do this with a
setup.sh script handled by the tooltool wrapper shell script, but it quickly
became cumbersome to handle combinations of multiple tooltool packages
requiring each their own setup. Back then, however, the setup was usually
limited to unpacking, which is now handled by tooltool itself.

Anyways, learning from this past experience, it appears better to have each
package handle its own setup separately, without having to deal with the
various combinations that may be requires if multiple packages need their
own setup.

This will allow replacing the initialization stuff from mozconfig.gtk with a setup script in the tooltool package itself.
Assignee: nobody → mh+mozilla
Flags: needinfo?(mshal)
Attachment #8640334 - Flags: review?(mshal)
Note that this sort of change makes me think tooltool would be better in-tree.
Blocks: 1188780
(In reply to Mike Hommey [:glandium] from comment #1)
> We kind of have setup.sh that tooltool runs after unpacking, but we've moved
> away from this because of the maintenance burden, especially when multiple
> packages need a setup, because then you have to maintain several such files,
> with different snippets, and updating becomes a giant PITA.

Yeah, and I think that will go away entirely soon.

> How about adding an entry to tooltool manifests saying something like setup:
> "setup.sh", in which case tooltool would execute the setup.sh script *in the
> tooltool package*?

That seems reasonable to me, even given how much of a pain setup.sh has been. Presumably this would just be added as part of build-gtk3.sh?

In any case, I think dustin is better suited for review since he has done a lot of tooltool work recently.
Attachment #8640334 - Flags: review?(mshal) → review?(dustin)
Comment on attachment 8640334 [details] [diff] [review]
Add a setup script field in tooltool manifests

Agreed re moving to tree - bug 1188985

The patch looks good, but needs corresponding updates to test_tooltool.py
Attachment #8640334 - Flags: review?(dustin) → feedback+
Attachment #8640334 - Attachment is obsolete: true
Attachment #8640913 - Flags: review?(dustin)
Comment on attachment 8640913 [details] [diff] [review]
Add a setup script field in manifests

The unit tests pass now, but with incomplete coverage:
  http://people.v.igoro.us/~dustin/tooltool-coverage/tooltool.html
The full automated tests are done by `validate.sh` which, among other things, requires 100% coverage.

Sorry to bump this back again!  The repo's set up with Travis-CI which would have helpfully told you these things in a Github PR.

The coverage serves a useful purpose -- at least for the moment, deploying tooltool is difficult and a failure can mean a rather lengthy outage while we re-deploy the earlier version.  Coverage != correctness, but it's close.
Attachment #8640913 - Flags: review?(dustin)
Note this changes the behavior when setup is set but unpack is not set, because it's easier to test those conditions.
Attachment #8640913 - Attachment is obsolete: true
Comment on attachment 8641499 [details] [diff] [review]
Add a setup script field in tooltool manifests

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

Looks great.  Thanks for the multiple go-rounds of review.
Attachment #8641499 - Flags: review+
Can you land this, I don't have access to https://github.com/mozilla/build-tooltool
Merged.  Now to get it deployed.
Depends on: 1189947
Depends on: 1189948
Depends on: 1189949
We could wait for bug 1188985
Eh, two of the three bugs are r? already and bug 1188985 is un-owned.  Best to deploy changes when they land, anyway, lest the next person to deploy be surprised.
Depends on: 1190860
Blocks: 1242641
Blocks: 1245828
In the end this was fixed in bug 1242641
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1242641
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in before you can comment on or make changes to this bug.