downthemall / anticontainer

DownThemAll! AntiContainer (Extension to a Firefox, Seamonkey extension)
Mozilla Public License 2.0
93 stars 41 forks source link

Updating the build process #60

Closed poke closed 12 years ago

poke commented 12 years ago

Hey @nmaier, I have been working on improving the build process for myself lately and since I have been running very well with them, I wanted to share them so we maybe get them merged in. However as you are still the one that makes the final builds, I wanted to make sure that it won’t break the process for you; hence this code review request.

The changes started with simple adjustments to get Python 3 support in the scripts, as that’s what I’m primarily using. Since I didn’t want to run Ant for every single change, I hooked up the plugin generation within my editor. One of those unproblematic changes already made it into master: 3775e7c400.

As I was working on the plugin generation scripts I also made some improvements which justified a separate file, as I was also merging it with the filter generation (to run through the plugins only once).

While doing that I also changed the way the filters are saved. Originally it would only write them to the generated xpi/ folder, but as the combined plugins are globally stored as well, I thought it would make more sense to do that for the filters as well, and provide these default preferences to the development environment as well.

One other thing was to speed up the “snapshot” generation for the nightlies I have been publishing to the repository with most changes. I’ve been able to completely integrate that into the Ant build though, which now gets the SHA1 from HEAD and makes a nice file name (and hopefully works across platforms).

And finally, I made the editing process for sandboxes way easier. It always bothered me having to decode and encode sandbox plugins for every change, although I’ve already been doing that with a JavaScript tool to speed it up. Now we simply have to change the JavaScript files within the sandboxes/ directory and run the build.

Again, these are just suggestions on improving the build process in general. If you have any problems with anything, I’m fine to keep using those tools for me personally, but I thought it would make sense to make it available for everybody. If you have further suggestions or want me to adjust some things (especially if something breaks), then of course I’ll be happy to adjust those in whatever way necessary.

Anyway, thanks for listening, and I’m happy to hear feedback :)

nmaier commented 12 years ago

Except for the nit this LGTM

nmaier commented 12 years ago

Oh, and your git-author seems unknown

poke commented 12 years ago

Except for the nit this LGTM

Good to hear – whatever “nit” exactly means though ^^ – I’ll fix and push that then later.

Oh, and your git-author seems unknown

That’s weird? Looking at the commits it does show the correct author content though, and GitHub recognizes me as well. Maybe there is something wrong on your end?

poke commented 12 years ago

Oh, I see now what you mean! It didn’t recognize me in the pull request view. Looks like a bug though…

poke commented 12 years ago

Okay, I just spotted some issue, I missed before: Because the dtaac.js preferences file contains the filter preferences as well, Firefox seems to ignore the generated filters.js and just uses the – empty – filter settings from before. I’m not sure why this does not happen in the released XPI, but it does happen when using the extension from the working directory and the build/xpi/ directory.

Looking at that made me wonder; do we actually want to include the dtaac.js file? Was there an initial reason to disable the imagefap plugin? And looking at 272f56a859, I guess the addition of the filters to the dtaac.js was to have those settings for the development environment, right? Do you know why the multiple definitions work for the final XPI, but not when used as a directory?

nmaier commented 12 years ago

It is mere coincidence what pref()s are parsed last. It relies on the enumeration of the "directory", may it be an OS enumeration or ZipReader enumeration. My guess is that the ZipReader returns dtaac.js, filters.js while the OS returns it the other way round (by coincidence).

You better re-merge those two files. Having a single prefs file will make the startup slightly more efficient.

The imagefap filter is disabled due to several reasons, the most important one because AMO wants it (as a concession to the no-porn rule). The only alternative is dropping that thing altogether.... However back when it was added, it was the single most requested filter...

poke commented 12 years ago

Okay, I updated the commits, implemented some filter merging – which won’t work for the workspace version, as Git wouldn’t like changing the file over and over ;) – and merged that in: aa2a6bc7f2.

However I immediately had to disable the filter generation again, as I’ll explain in a follow-up issue; this is not related to the build process update though, it already existed before, but simply never occurred.