1479599 - Pure webextension system addons are not packed into xpis during packaging

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Andrew Swan [:aswan]]

I've been working on converting formautofill from a bootstrapped extension to a webextension over in bug 1449055, but the extension no longer gets packed into an xpi which causes a slew of failures.
I believe the problem is here:
https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/python/mozbuild/mozpack/packager/__init__.py#267-272

that is, the logic there just looks for an install.rdf file and assumes that the enclosing directory is an extension if it finds one.  Just adding a check for manifest.json seems risky (partly because that name is quite generic, partly because we still have extensions like screenshots that embed a webextension within a bootstrapped extension).

(while we're at it, mozbuild still has support for unpacked extensions that I think we can safely delete now)

Comment 1

[Andrew Swan [:aswan]]

glandium, can you give some guidance for how this ought to work?

Comment 2

[Mike Hommey [:glandium]]

How does one detect a webextension?

Comment 3

[Andrew Swan [:aswan]]

We could do something simple like parse the manifest and look for a manifest_version property.  But I fear that would break for screenshots which (currently) included a webextension embedded within a bootstrapped extension.

What about just adding something like "PACK_XPI = True" to moz.build?

Comment 4

[Mike Hommey [:glandium]]

Well, a webextension within a bootstrapped extension is detectable too.

Comment 5

[Andrew Swan [:aswan]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Well, a webextension within a bootstrapped extension is detectable too.

Sorry, not sure what you're proposing.  You want to keep the current technique of detecting extensions from inside SimplePackager.add_file(), and add something like this?

```
if path == 'manifest.json' and verify_this_is_a_webextension_manifest(path) and this_is_not_an_embedded_webextension():
  self._addons[path] = True
```

To reliably check for an embedded webextension we would have to walk back up the directory hierarchy looking for a bootstrapped extension, though embedded extensions should be going away soon so we could probably get away with something like only looking at the parent directory...

Comment 6

[Mike Hommey [:glandium]]

(In reply to Andrew Swan [:aswan] from comment #5)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Well, a webextension within a bootstrapped extension is detectable too.
> 
> Sorry, not sure what you're proposing.  You want to keep the current
> technique of detecting extensions from inside SimplePackager.add_file(), and
> add something like this?
> 
> ```
> if path == 'manifest.json' and verify_this_is_a_webextension_manifest(path)
> and this_is_not_an_embedded_webextension():
>   self._addons[path] = True
> ```

```
if path == 'manifest.json' and verify_this_is_a_webextension_manifest(path):
    self._addons[path] = True
```

would be enough, and then, modify get_bases to eliminate addons-within-addons.

also, get_bases probably wouldn't return a webextension as a base if it doesn't have a chrome.manifest, so you'd need to account for that too.

I'd suggest starting by writing a unit test in python/mozbuild/mozpack/test/test_packager.py.

Comment 7

[Andrew Swan [:aswan]]

Attachment: Bug 1479599 Part 1: Remove support for unpacked extensions

Review commit: https://reviewboard.mozilla.org/r/260704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260704/

Comment 8

[Andrew Swan [:aswan]]

Attachment: Bug 1479599 Recognize webextensions in packager

Review commit: https://reviewboard.mozilla.org/r/260706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260706/

Comment 9

[Andrew Swan [:aswan]]

(In reply to Mike Hommey [:glandium] from comment #6)
> also, get_bases probably wouldn't return a webextension as a base if it
> doesn't have a chrome.manifest, so you'd need to account for that too.

this appears to be already handled here:
https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/python/mozbuild/mozpack/packager/__init__.py#318-320

I also did some minimal cleanup of the now obsolete support for unpacked extensions in packager.  I think there's some further cleanup that could be done (ie, binary-components are no longer supported anywhere), but I didn't have the time to figure out and update all the related tests.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8996603 [details]
Bug 1479599 Part 1: Remove support for unpacked extensions

https://reviewboard.mozilla.org/r/260704/#review267962

I'd rather not remove those. It's a purposeful feature that this code can still handle older versions of Firefox (which allows to unpack them and fiddle with them without having to figure out what the right version of the code is).

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8996604 [details]
Bug 1479599 Recognize webextensions in packager

https://reviewboard.mozilla.org/r/260706/#review267964

::: python/mozbuild/mozpack/packager/__init__.py:276
(Diff revision 1)
> +        parent = [dir for dir in self._addons.keys()
> +                  if path.startswith(dir)]

Use mozpath.basedir(path, self._addons)

(In reply to Andrew Swan [:aswan] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > also, get_bases probably wouldn't return a webextension as a base if it
> > doesn't have a chrome.manifest, so you'd need to account for that too.
> 
> this appears to be already handled here:
> https://searchfox.org/mozilla-central/rev/
> 033d45ca70ff32acf04286244644d19308c359d5/python/mozbuild/mozpack/packager/
> __init__.py#318-320
> 

Yes and no. This only works if the files are added in the right order, and that's not guaranteed.

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8996604 [details]
Bug 1479599 Recognize webextensions in packager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260706/diff/1-2/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8996604 [details]
Bug 1479599 Recognize webextensions in packager

Comment 11 still applies.

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8996604 [details]
Bug 1479599 Recognize webextensions in packager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260706/diff/2-3/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8996604 [details]
Bug 1479599 Recognize webextensions in packager

https://reviewboard.mozilla.org/r/260706/#review269058

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6005ad88309
Recognize webextensions in packager r=glandium

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e6005ad88309

Comment 18

[u656541]

For what is worth, I got this:

==> Entering fakeroot environment...
/usr/bin/makepkg -F -s --noprepare --force --noextract --
==> Starting package()...
 0:01.68 /usr/bin/make -C . -j4 -s -w install
 0:01.68 Environment: {'HISTTIMEFORMAT': '%F %T ', 'MAKEPKG_FAIL_IF_REPO_UPDATE_FAILS': '1', 'LESS_TERMCAP_md': '\x1b[01;38;5;75m', 'LESS': '-+X -R -+S -+F -+E -I -M --shift 5', 'SOURCE_DATE_EPOCH': '1586890070', 'mainreponame': 'q1q', 'XDG_SESSION_TYPE': 'tty', 'FAKEROOTKEY': '1319733720', 'SHELL': '/bin/bash', 'GIT_SSH': '/home/user/bin/sshgit', 'XDG_DATA_DIRS': '/home/user/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share:/usr/share', 'CLICOLOR_FORCE': '1', 'HISTSIZE': '-1', 'TEXTDOMAIN': 'pacman-scripts', 'CPPFLAGS': '-pipe -march=native -Wno-trigraphs -fno-schedule-insns2 -fno-delete-null-pointer-checks -mtune=native -fomit-frame-pointer -O2 -D_FORTIFY_SOURCE=2 -DQT_FORCE_ASSERTS -ggdb -rdynamic', 'INVOCATION_ID': '778d24a46aaf46f093d309c88529ce07', 'XDG_RUNTIME_DIR': '/run/user/1000', 'MOZ_SOURCE_REPO': 'https://hg.mozilla.org/mozilla-central', 'CCACHE_NOCOMPRESS': '1', 'VAGRANT_DISABLE_VBOXSYMLINKCREATE': '1', 'FREETYPE_PROPERTIES': 'truetype:interpreter-version=38', 'DEPOT_TOOLS_METRICS': '0', 'RUST_SRC_PATH': '/home/user/build/2nonpkgs/rust.stuff/rust/rust/src', 'XDG_SESSION_ID': 'c1', 'DBUS_SESSION_BUS_ADDRESS': 'unix:abstract=/tmp/dbus-XOi7Nn7GdF,guid=1ab49210a4071be0ba5efb7d5e957ae8', 'LESS_TERMCAP_ue': '\x1b[0m', 'MOZ_PLUGIN_PATH': '/24905', 'RUST_BACKTRACE': '0', 'MC_PS1': '\\W \\$ ', 'XZ_DEFAULTS': '--threads=0', 'MACH_MAIN_PID': '2373789', 'MAIL': '/var/spool/mail/user', 'LS_COLORS': 'rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.zst=01;31:*.tzst=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.wim=01;31:*.swm=01;31:*.dwm=01;31:*.esd=01;31:*.jpg=01;35:*.jpeg=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.webp=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:', 'GIT_SSH_VARIANT': 'ssh', 'GOPATH': '/tmp/gopath', 'INSTALL_SDK': '', 'CCACHE_TEMPDIR': '/tmp', 'USER': 'user', 'PROMPT_COMMAND': '__earlyec=("${PIPESTATUS[@]}")', 'XDG_VTNR': '1', 'GIT_NO_REPLACE_OBJECTS': '1', u'PYTHONUNBUFFERED': u'1', 'GTK_THEME': 'Breeze:Dark', 'HG': '/usr/bin/hg', 'PS4': '+ ', 'XAUTHORITY': '/home/user/.Xauthority', 'JOURNAL_STREAM': '9:5916', 'SESSION_MANAGER': 'local/Z575:@/tmp/.ICE-unix/1372,unix/Z575:/tmp/.ICE-unix/1372', 'SHLVL': '8', 'DISPLAY': ':0.0', 'WINDOWID': '46331972', 'CHOST': 'x86_64-pc-linux-gnu', 'EDITOR': 'vim', 'CCACHE_SLOPPINESS': 'include_file_mtime,file_stat_matches,include_file_ctime,file_stat_matches_ctime', 'LESS_TERMCAP_us': '\x1b[04;38;5;200m', 'FUNCNEST': '30000', 'CARGO_EXPAND_NO_RUN_NIGHTLY': '1', 'CFG_DISABLE_CROSS_TESTS': '1', 'GTK2_RC_FILES': '/home/user/.gtkrc-2.0', 'TMPDIR': '/tmp', 'CCACHE_NOHARDLINK': '1', 'CCACHE_BASEDIR': '/home/user/build/', 'DESTDIR': '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/pkg/firefox-hg', 'CCACHE_NOHASHDIR': '1', 'COLORTERM': 'truecolor', 'PAGER': 'less', 'COMMAND_MODE': 'legacy', 'serialize': '', 'CXXFLAGS': '-pipe -march=native -Wno-trigraphs -fno-schedule-insns2 -fno-delete-null-pointer-checks -mtune=native -fomit-frame-pointer -O2 -D_FORTIFY_SOURCE=2 -DQT_FORCE_ASSERTS -ggdb -rdynamic', 'MY_GIT_USERNAME': 'none', 'HOME': '/home/user', 'LD_LIBRARY_PATH': '/usr/lib/libfakeroot:/usr/lib64/libfakeroot:/usr/lib32/libfakeroot', 'LANG': 'en_US.UTF-8', 'XFWM4_LOG_FILE': '/tmp/xfwm4-debug-%d.log', 'MY_GIT_EMAILADDRESS': 'none@Z575', 'TERMINFO': '/home/user/.terminfo', 'VTE_VERSION': '6100', 'YAOURT_COLORS': 'nb=1:pkg=1:ver=1;32:lver=1;45:installed=1;42:grp=1;34:od=1;41;5:votes=1;44:dsc=0:other=1;35', 'FAKED_MODE': 'unknown-is-root', 'GIT_PAGER': 'less', 'GPG_TTY': '/dev/pts/18', 'MOZBUILD_STATE_PATH': '/tmp/.mozbuild', 'CSCOPE_DB': '/home/user/cscope/', 'MOZ_MAKE_FLAGS': '-j6 --no-keep-going ', 'PYTHON': '/usr/bin/python2', 'LOGNAME': 'user', 'XDG_SEAT': 'seat0', 'PATH': '/home/user/bin/binprio:/usr/local/sbin:/usr/local/bin:/usr/bin:/var/lib/flatpak/exports/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/home/user/bin:/home/user/bin/oldbin:/opt/depot_tools/:/home/user/build/2nonpkgs/rust.stuff/chars/target/debug:/home/user/build/2nonpkgs/rust.stuff/chars/target/release:/home/user/.cargo/bin:/var/lib/flatpak/exports/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl', 'MAKEFLAGS': '-j6 --no-keep-going ', 'SSH_AGENT_PID': '1441', 'HISTCONTROL': 'ignorespace', 'TERM': 'xterm-256color', 'LESS_TERMCAP_me': '\x1b[0m', 'hostname': 'Z575', 'LESS_TERMCAP_mb': '\x1b[01;31m', 'WINDOWPATH': '1', 'LDFLAGS': '-Wl,-O1,--sort-common,--as-needed,-z,relro', 'HISTFILESIZE': '-1', 'XDG_SESSION_CLASS': 'user', 'PROMPT_DIRTRIM': '2', 'XZ_OPT': '--threads=0', 'SSH_AUTH_SOCK': '/tmp/ssh-ZTe5hK5mc3fu/agent.1440', 'TEXTDOMAINDIR': '/usr/share/locale', 'MACH_STDOUT_ISATTY': '1', u'MACH': u'1', 'LD_PRELOAD': '/usr/lib64/libfakeroot/libfakeroot.so', 'DONT_MOUNT_BOOT': '1', 'OLDPWD': '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src', 'LESS_TERMCAP_se': '\x1b[0m', 'PWD': '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg', 'CFLAGS': '-pipe -march=native -Wno-trigraphs -fno-schedule-insns2 -fno-delete-null-pointer-checks -mtune=native -fomit-frame-pointer -O2 -D_FORTIFY_SOURCE=2 -DQT_FORCE_ASSERTS -ggdb -rdynamic', 'LESS_TERMCAP_so': '\x1b[01;33m', 'NINJA_STATUS': '\n\x1b[42m[%u/%r/%f/%t] %es\x1b(B\x1b[m [remaining/running/finished/totalthissession] elapsed\n\x1b];[%u/%r/%f/%t] %es [remaining/running/finished/totalthissession] elapsed\x07'}
 0:01.77 make: Entering directory '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu'
 0:01.85 make[1]: Entering directory '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/browser/installer'
 0:03.69 Traceback (most recent call last):
 0:03.69   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/toolkit/mozapps/installer/packager.py", line 250, in <module>
 0:03.69     main()
 0:03.69   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/toolkit/mozapps/installer/packager.py", line 199, in main
 0:03.69     preprocess_manifest(sink, args.manifest, defines)
 0:03.69   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 216, in preprocess_manifest
 0:03.70     preprocess(manifest, PackageManifestParser(sink), defines)
 0:03.70   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 207, in preprocess
 0:03.70     pp.do_include(input)
 0:03.70   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozbuild/preprocessor.py", line 841, in do_include
 0:03.70     self.handleLine(l)
 0:03.70   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozbuild/preprocessor.py", line 601, in handleLine
 0:03.70     self.write(aLine)
 0:03.70   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozbuild/preprocessor.py", line 483, in write
 0:03.70     self.out.write(filteredLine)
 0:03.70   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 195, in write
 0:03.70     self._parser.handle_line(str)
 0:03.71   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 180, in handle_line
 0:03.71     self._sink.add(self._component, str)
 0:03.71   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 427, in add
 0:03.71     self.packager.add(dest, f)
 0:03.71   File "/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/python/mozbuild/mozpack/packager/__init__.py", line 295, in add
 0:03.71     if isinstance(parsed, dict) and 'manifest_version' in parsed:
 0:03.71 UnboundLocalError: local variable 'parsed' referenced before assignment
 0:03.78 make[1]: *** [/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/toolkit/mozapps/installer/packager.mk:25: stage-package] Error 1
 0:03.78 make[1]: Leaving directory '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu/browser/installer'
 0:03.78 make: *** [/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/browser/build.mk:15: install] Error 2
 0:03.78 make: Leaving directory '/home/user/build/1packages/4used/firefox-hg/makepkg_pacman/firefox-hg/src/firefox-hg/obj-x86_64-pc-linux-gnu'
==> ERROR: A failure occurred in package().
    Aborting...

ie. json.loads() threw, so parsed wasn't defined...

Comment 19

[u656541]

Attachment: dontignore_invalid_manifest_json.patch

this is the patch that I'm using now.
I had modified that manifest.json (as part of a different patch) and thus had a // comment on a line, which json.loads() threw on (rightfully), but otherwise loading that manifest.json(as part of an .xpi aka zip) worked in about:addons -> Themes even with that comment.