Closed Bug 1346643 Opened 7 years ago Closed 7 years ago

Make it possible to package MozillaBuild without requiring a special builder VM

Categories

(Firefox Build System :: MozillaBuild, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

Attachments

(4 files, 3 obsolete files)

MozillaBuild has a hacked together build system that's been built up around a specially-configured Windows Server 2003 builder VM. This makes it very hard for contributors to test their work and limits the ability for people to take ownership. It's also actively blocking any hopes of ever doing a package that includes 64-bit binaries of critical components.

Some work was already done during the 2.0 development cycle to eliminate the need for customized tooling, but there's still more to be done. Since it looks like we're still going to have to care about MozillaBuild for the foreseeable future, let's finish the job.
Summary: Make it possible to package MozillaBuild with requiring a special builder VM → Make it possible to package MozillaBuild without requiring a special builder VM
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-build/rev/14583629fb6f
Don't build autoconf-2.13 and make-3.81.90 from source during packaging and use prebuilt binaries instead.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
There's no reason to be building autoconf-2.13 and make-3.81.90 from source during packaging every time as it's pretty much certain that they'll never change and they depend on an old version of MSYS/mingw to be built. Let's just use pre-built binaries instead.
https://hg.mozilla.org/mozilla-build/rev/14583629fb6f
...thanks pulsebot.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Depends on: 1346659
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-build/rev/823f301f8ce8
Remove make-msys.patch since it's no longer needed.
I need to clean these up a bit still, but attaching the WIP patches I've got. At this point, I can build a fully functional installer on my own machine without the need for the builder VM \m/
This is the big one. Still needs some cleaning up, but most importantly, it all works!
These are really exciting - getting very close to removing the last major barrier of entry for people wanting to contribute to MozillaBuild. Once these get finished up and landed, we can move on to the next big project - making it 64-bit :D
Attachment #8868841 - Flags: review?(gps)
Attachment #8868842 - Flags: review?(gps)
I'm....not a python expert. I did my best to write good, clean code here, but I'm very much open to any improvements you might suggest. I've thoroughly audited the results of this script to ensure that it produces the same end result as the previous combination of scripts.
Attachment #8868843 - Attachment is obsolete: true
Attachment #8869295 - Flags: review?(gps)
In case it helps, this is what packageit.py looks like with these patches applied.
Comment on attachment 8868841 [details] [diff] [review]
Remove make-mozillabuild.bat in favor of packageit.py

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

::: packageit.py
@@ +48,5 @@
>  print("Output location: " + stagedir)
>  
> +# Remove the old stage directory if it's already present.
> +if exists(stagedir):
> +    check_call(["cmd.exe", "/C", "rmdir /S /Q %s" % stagedir])

While you're here, you might as well replace with shutil.rmtree()
Attachment #8868841 - Flags: review?(gps) → review+
Comment on attachment 8868842 [details] [diff] [review]
Use 7-Zip to extract KDiff3 rather than running the SFX installer directly

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

That is really unfortunate!
Attachment #8868842 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #12)
> While you're here, you might as well replace with shutil.rmtree()

I had looked at it, but I found that cmd.exe was more forgiving of the directory being in use than python was. Is there a way to convince Python to delete the directory anyway?
Comment on attachment 8869295 [details] [diff] [review]
Port packageit.sh over to packageit.py

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

This seems mostly reasonable! Yay for one fewer shell script.

::: packageit.py
@@ +20,5 @@
>  
> +from os import chdir, chmod, mkdir, remove, rename, walk
> +from os.path import abspath, dirname, exists, join, split
> +from shutil import copyfile, copytree
> +from subprocess import check_call, check_output

Nit: it isn't common to alias the os or os.path symbols because many of them are ambiguous. Not a huge deal. But it looks odd to experienced Python programmers.

@@ +99,3 @@
>  python_installer = "python-2.7.13.msi"
> +check_call(["msiexec.exe", "/q", "/a", join(sourcedir, python_installer),
> +            "TARGETDIR=" + python27_dir])

Eh? I know for a fact the Python installer can run in a mode that doesn't pollute the system. Is this a matter of passing a new flag to the installer?

@@ +192,5 @@
> +
> +# Copy yasm to the stage directory.
> +print "Staging yasm 1.3.0..."
> +if not exists(join(pkgdir, "yasm")):
> +    mkdir(join(pkgdir, "yasm"))

Strictly speaking this is an anti-pattern: you should just mkdir then catch the exception if the directory exists. But for this use case it is fine.

@@ +200,5 @@
> +print "Extracting MSYS components..."
> +msysdir = join(pkgdir, "msys")
> +if not exists(msysdir):
> +    mkdir(msysdir)
> +chdir(msysdir)

Hmm. Shouldn't need to do chdir() here. check_call() accepts a cwd argument to launch the process in. Use that instead.

@@ +206,5 @@
> +    print "    " + archive
> +    check_call(["tar", "--lzma", "--force-local", "-xf", join(sourcedir, "msys", archive)])
> +
> +# mktemp.exe extracts as read-only, which breaks manifest embedding later.
> +chmod(join(msysdir, r"bin\mktemp.exe"), 755)

I'm pretty sure this doesn't do what you think it does. 0755 is what you need to engage octal notation.

@@ +283,5 @@
> +print "Packaging everything up into the installer..."
> +copyfile(join(sourcedir, "license.rtf"), join(stagedir, "license.rtf"))
> +copyfile(join(sourcedir, "installit.nsi"), join(stagedir, "installit.nsi"))
> +# Write the MozillaBuild version number to version.nsi
> +with open(join(sourcedir, "version.nsi"), 'r') as version_old, open(join(stagedir, "version.nsi"), 'wb') as version_new:

Opening the same file at the same time in different modes - I'm surprised this works as expected. You should open and buffer the file first, the close and reopen in write mode.
Attachment #8869295 - Flags: review?(gps)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #12)
> > While you're here, you might as well replace with shutil.rmtree()
> 
> I had looked at it, but I found that cmd.exe was more forgiving of the
> directory being in use than python was. Is there a way to convince Python to
> delete the directory anyway?

It is a fundamental limitation of Windows that you can't delete a file if there is an open handle to it. In the case of a directory, a process's current directory counts as an open file.

Does that help?
Only in that cmd.exe appears to force-close that handle - i.e. if you have an Explorer window open to that directory, it basically boots you out of it and removes it anyway.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Only in that cmd.exe appears to force-close that handle - i.e. if you have
> an Explorer window open to that directory, it basically boots you out of it
> and removes it anyway.

TIL!
(In reply to Gregory Szorc [:gps] from comment #12)
> While you're here, you might as well replace with shutil.rmtree()

I've added a comment at least explaining the rationale for sticking with cmd.exe here.

(In reply to Gregory Szorc [:gps] from comment #15)
> Nit: it isn't common to alias the os or os.path symbols because many of them
> are ambiguous. Not a huge deal. But it looks odd to experienced Python
> programmers.

Done. Though I left join alone because it's pretty heavily used throughout and it seemed unambiguous still as to what it was doing.

> Eh? I know for a fact the Python installer can run in a mode that doesn't
> pollute the system. Is this a matter of passing a new flag to the installer?

At least with the MSI installer, I'm not aware of any better options, and I do recall spending time investigating it at one point. I'll be changing this over to the 64-bit installer soon anyway and I can take another look then if you want. For now, though, this is using the same logic that was already in place so I'd prefer to take this to a follow-up if that's OK with you.

> Hmm. Shouldn't need to do chdir() here. check_call() accepts a cwd argument
> to launch the process in. Use that instead.

Done, both here and during the installer packaging at the bottom.

> I'm pretty sure this doesn't do what you think it does. 0755 is what you
> need to engage octal notation.

I did appear to work! But I'm all for correctness, so fixed :)

> Opening the same file at the same time in different modes - I'm surprised
> this works as expected. You should open and buffer the file first, the close
> and reopen in write mode.

It's opening the file from the source dir and writing it to the stage dir. So I think we're fine here? If there's still an issue, I can probably copy version.nsi over beforehand and use fileinput inplace editing otherwise. For now, I've at least added a better comment.
Attachment #8869295 - Attachment is obsolete: true
Attachment #8870241 - Flags: review?(gps)
Attachment #8869296 - Attachment is obsolete: true
Comment on attachment 8870241 [details] [diff] [review]
Port packageit.sh over to packageit.py

Ignore the version.nsi changes in this - it was some stale state that crept into my working directory while I was working on this. I've removed it from the patch locally and verified that the script isn't touching anything in the source directory when it runs.
Comment on attachment 8870241 [details] [diff] [review]
Port packageit.sh over to packageit.py

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

This all seems pretty reasonable.

::: packageit.py
@@ +118,2 @@
>  # Install hgwatchman - TEMPORARILY COMMENTED OUT DUE TO BUSTAGE
> +#check_call([join(python27_dir, "python.exe"), "-m", "pip", "install", "hgwatchman"])

Just delete these lines since Mercurial support for watchman is now built into Mercurial itself as part of the fsmonitor extension.

@@ +120,5 @@
> +# Install virtualenv.
> +check_call([join(python27_dir, "python.exe"), "-m", "pip", "install", "virtualenv"])
> +# Install Mercurial and copy mercurial.ini to the python dir so Mercurial has sane defaults.
> +check_call([join(python27_dir, "python.exe"), "-m", "pip", "install", "mercurial"])
> +copyfile(join(sourcedir, "mercurial.ini"), join(python27_dir, "mercurial.ini"))

We'll need to update mercurial.ini for Mercurial 4.2 compatibility since 4.2 enabled color and pager by default and changes behavior if the extensions are installed. Please make sure there is a bug on file tracking our audit before the MozillaBuild release.
Attachment #8870241 - Flags: review?(gps) → review+
Thanks for the review! I actually was planning to file a new bug for fsmonitor specifically so we can take a look at updating the copy of watchman that we ship as well. Figured I'd make the related changes there while I was at it.
Keywords: leave-open
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-build/rev/58a136c31a55
Remove make-mozillabuild.bat in favor of packageit.py. r=gps
https://hg.mozilla.org/mozilla-build/rev/3db01d6552f7
Use 7-Zip to extract KDiff3 rather than running the SFX installer directly. r=gps
https://hg.mozilla.org/mozilla-build/rev/0e82ec825f21
Port packageit.sh over to packageit.py. r=gps
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: mozilla.org → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: