Start on Mach package #11210

edunham · 2016-05-16T19:59:27Z

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

./mach build -d does not report any errors
- these changes don't touch anything that mach build touches>
./mach test-tidy --faster does not report any errors
- Tidy errors on some dependencies that I think we'll need for real package but aren't using for android
These changes address #9918 (github issue number if applicable).

Either:

There are tests for these changes OR
These changes do not require tests because I don't think Mach has tests yet?

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

This change is

highfive · 2016-05-16T19:59:32Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/mach_bootstrap.py, python/servo/package_commands.py, python/servo/post_build_commands.py

nox · 2016-05-16T21:21:40Z

This fails tidy.

$ ./mach test-tidy --no-progress
Checking files for tidiness...
./python/servo/package_commands.py:12: F401 'glob' imported but unused
./python/servo/package_commands.py:16: F401 'copy2' imported but unused
./python/servo/package_commands.py:16: F401 'copytree' imported but unused
./python/servo/package_commands.py:16: F401 'rmtree' imported but unused
./python/servo/package_commands.py:26: F401 'call' imported but unused
./python/servo/package_commands.py:26: F401 'check_call' imported but unused
./python/servo/package_commands.py:28: E302 expected 2 blank lines, found 1
./python/servo/post_build_commands.py:26: F401 'BuildNotFound' imported but unused

edunham · 2016-05-16T22:44:21Z

@nox yep, I noted that in the amazing PR template which reminded me to run Tidy. PR is up mainly so @larsberg and I can point at it and discuss.

nox · 2016-05-16T22:45:45Z

@edunham Oh sorry. :( I'm so accustomed to seeing this snippet of text that I don't notice unusual things in it anymore...

frewsxcv · 2016-05-18T02:52:15Z

python/servo/package_commands.py

+        apk_path = binary_path + ".apk"
+        if not path.exists(apk_path):
+            result = Registrar.dispatch("package", context=self.context, release=release, dev=dev)
+            if result is not 0:


The is binary operator is used to check references. i.e., (IIRC) it's possible for multiple instances of 0 to exist in memory, so this is not guaranteed to always pass. Whereas singletons, like None, True, and False only exist once in memory so is is okay. So this should maybe be an != operator instead

larsbergstrom · 2016-05-26T15:29:00Z

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

larsbergstrom · 2016-05-26T15:29:41Z

@edunham If you address the comment from @frewsxcv, we can land this. Or, you can add the initial packaging steps for mac/linux/etc. and we can go from there. Whichever you'd prefer!

highfive · 2016-05-31T18:14:47Z

New code was committed to pull request.

edunham · 2016-05-31T18:20:35Z

@larsbergstrom Latest commits do naive packaging for OSX/Linux, including creating ./runservo.sh to invoke Servo with browserhtml.

Next steps:

Implement a blacklist or whitelist to nuke unneeded files from the release directory -- I've tried messing around with it a little but don't have a deep enough understanding of how Servo works to make good guesses on what's safe to eliminate. Arguably, the presence of any unneeded files in the release directory is actually an error in the build step anyways. edit: done in next commit after this comment
Tweak tarball naming if desired -- it's currently of the form 2016-05-31-1053-servo-tech-demo.tar.gz. I've included an hour/minute timestamp for now to assist with debugging -- I imagine we'll run into circumstances where we wonder whether a tarball was built before or after a specific patch landed. Longer-term, we may want to either take a name suffix argument to mach package or munge the names during upload, to differentiate nightlies from other releases.
Make the builders run it & upload the artefacts


        Address frewsxcv's comment on unreliability of `is`

servo#11210 (comment)

highfive · 2016-05-31T23:37:55Z

New code was committed to pull request.

edunham · 2016-05-31T23:40:44Z

@larsbergstrom It's all squashed up and ready for final review :)

larsbergstrom · 2016-06-01T14:35:36Z

python/servo/package_commands.py

+@CommandProvider
+class PackageCommands(CommandBase):
+    @Command('package',
+             description='Package Servo (currently, Android APK only)',


This comment is now incorrect :-)

larsbergstrom · 2016-06-01T14:35:52Z

After fixing the one total nit, please squash and r=me!

jdm · 2016-06-02T20:34:51Z

Checking files for tidiness...

./python/servo/package_commands.py:30: E302 expected 2 blank lines, found 1

./python/servo/package_commands.py:34: E261 at least two spaces before inline comment

./python/servo/package_commands.py:36: E302 expected 2 blank lines, found 1

larsbergstrom · 2016-06-03T18:25:31Z

@edunham Please fix tidy and r=me.

aneeshusa · 2016-06-03T20:57:01Z

python/servo/package_commands.py

+             description='Install Servo (currently, Android only)',
+             category='package')
+    @CommandArgument('--release', '-r', action='store_true',
+                     help='Package the release build')


'Package' -> 'Install'

aneeshusa · 2016-06-03T20:57:15Z

python/servo/package_commands.py

+    @CommandArgument('--release', '-r', action='store_true',
+                     help='Package the release build')
+    @CommandArgument('--dev', '-d', action='store_true',
+                     help='Package the dev build')


'Package' -> 'Install'

aneeshusa · 2016-06-03T21:07:25Z

python/servo/package_commands.py

+            os.close(runservo)
+            print("Creating tarball")
+            tar_path = '/'.join(dir_to_package.split('/')[:-1]) + '/'
+            tar_path += time.strftime('%Y-%m-%d-%H%M') + "-servo-tech-demo.tar.gz"


Can we please use ISO 8601 format?

import datetime datetime.utcnow().replace(microsecond=0).isoformat()

Ideally this would also indicate that this is UTC time in the printed representation, but Python 2 doesn't have datetime.timezone.utc, and I don't think an extra dependency or writing a UTC tzinfo subclass is worth it..

aneeshusa · 2016-06-03T21:08:22Z

python/servo/package_commands.py

+            if result != 0:
+                return result
+
+        print(["adb", "install", "-r", apk_path])


Looks like a debugging print that can be removed.

I believe this is still used frequently when debugging the Android builder


        Create `mach package`

* Split package commands into their own file * Delete spurious files from build dir * Create runservo.sh to invoke servo with the right browserhtml incantation * Tar it all up with the date and time in the filename

highfive · 2016-06-03T21:47:35Z

New code was committed to pull request.

edunham · 2016-06-03T21:47:56Z

Latest changes fix tidy errors and switch to using ISO datetime format.

aneeshusa · 2016-06-03T23:26:26Z

I don't think the tarfile Python module has the right options to support reproducible tarball builds, for example I don't see any way to enforce file ordering. I'd really like to see us have reproducible builds in general, but now that it's June I understand the tech demo is right around the corner; if this is something we have time for now I can dig into it more, otherwise this can be changed after the demo.

edunham · 2016-06-06T20:18:02Z

@aneeshusa I'd like to get a basic package functionality landed so we can add it to the build system -- in the long run there are a lot of areas for improvement.

r? @larsbergstrom

larsbergstrom · 2016-06-06T20:19:53Z

@bors-servo r+

bors-servo · 2016-06-06T20:19:54Z

📌 Commit a1a8e08 has been approved by larsbergstrom

bors-servo · 2016-06-06T20:19:56Z

⌛ Testing commit a1a8e08 with merge bc52617...


        Auto merge of #11210 - edunham:mach-package, r=larsbergstrom

Start on Mach package Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [ ] `./mach build -d` does not report any errors - these changes don't touch anything that mach build touches> - [ ] `./mach test-tidy --faster` does not report any errors - Tidy errors on some dependencies that I think we'll need for real `package` but aren't using for android - [X] These changes address #9918 (github issue number if applicable). Either: - [ ] There are tests for these changes OR - [x] These changes do not require tests because I don't think Mach has tests yet? Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11210)

bors-servo · 2016-06-06T21:44:17Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows


        Add --android option to ./mach package

servo/servo#11210 recently updated `./mach package` to make it a first-class Mach command and add support for simple Linux packaging as well (in the form of tarballs). Android used to be the only packaging target supported, so a naked `./mach package` sufficed, but now we need to explicitly build the Android packaging.


        Auto merge of #401 - aneeshusa:add-android-option-to-mach-package, r=…

…larsbergstrom Add --android option to ./mach package servo/servo#11210 recently updated `./mach package` to make it a first-class Mach command and add support for simple Linux packaging as well (in the form of tarballs). Android used to be the only packaging target supported, so a naked `./mach package` sufficed, but now we need to explicitly build the Android packaging. Another step for servo/servo#10339.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/401)

highfive assigned nox May 16, 2016

highfive added the S-awaiting-review label May 16, 2016

larsbergstrom assigned larsbergstrom and unassigned nox May 16, 2016

nox added the S-fails-tidy label May 16, 2016

frewsxcv reviewed May 18, 2016
View changes

edunham force-pushed the edunham:mach-package branch from 337c117 to 2d69d2b May 31, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels May 31, 2016

edunham force-pushed the edunham:mach-package branch from 2d69d2b to c398399 May 31, 2016

edunham added a commit to edunham/servo that referenced this pull request May 31, 2016

Address frewsxcv's comment on unreliability of `is`

ab9584a

servo#11210 (comment)

edunham force-pushed the edunham:mach-package branch 4 times, most recently from 63d4672 to 79b983e May 31, 2016

larsbergstrom reviewed Jun 1, 2016
View changes

jdm added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Jun 1, 2016

highfive added the S-tests-failed label Jun 2, 2016

jdm removed the S-tests-failed label Jun 2, 2016

aneeshusa reviewed Jun 3, 2016
View changes

Create `mach package`

Loading status checks…

a1a8e08

* Split package commands into their own file * Delete spurious files from build dir * Create runservo.sh to invoke servo with the right browserhtml incantation * Tar it all up with the date and time in the filename

edunham force-pushed the edunham:mach-package branch from 6fe127c to a1a8e08 Jun 3, 2016

highfive added the S-awaiting-review label Jun 3, 2016

highfive removed the S-awaiting-review label Jun 6, 2016

highfive added the S-awaiting-merge label Jun 6, 2016

bors-servo merged commit a1a8e08 into servo:master Jun 6, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 6, 2016

bors-servo mentioned this pull request Jun 6, 2016

Fix ./mach --browserhtml #11543

Merged

aneeshusa mentioned this pull request Jun 22, 2016

Test packaging on android builder servo/saltfs#386

Merged

aneeshusa mentioned this pull request Jun 22, 2016

Add --android option to ./mach package servo/saltfs#401

Merged

aneeshusa mentioned this pull request Jul 1, 2016

Create Linux .tar.gz package archive deterministically #11981

Closed

servo / servo

Start on Mach package #11210

Start on Mach package #11210

edunham commented May 16, 2016 •

edited by larsbergstrom

highfive commented May 16, 2016

nox commented May 16, 2016

edunham commented May 16, 2016

nox commented May 16, 2016

This comment has been minimized.

larsbergstrom commented May 26, 2016

larsbergstrom commented May 26, 2016

highfive commented May 31, 2016

edunham commented May 31, 2016 •

edited

highfive commented May 31, 2016

edunham commented May 31, 2016

This comment has been minimized.

larsbergstrom commented Jun 1, 2016

jdm commented Jun 2, 2016

larsbergstrom commented Jun 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

highfive commented Jun 3, 2016

edunham commented Jun 3, 2016

aneeshusa commented Jun 3, 2016

edunham commented Jun 6, 2016

larsbergstrom commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 6, 2016

servo / servo

Join GitHub today

Start on Mach package #11210

Start on Mach package #11210

Conversation

edunham commented May 16, 2016 • edited by larsbergstrom

highfive commented May 16, 2016

nox commented May 16, 2016

edunham commented May 16, 2016

nox commented May 16, 2016

This comment has been minimized.

frewsxcv May 18, 2016 • edited

larsbergstrom commented May 26, 2016

larsbergstrom commented May 26, 2016

highfive commented May 31, 2016

edunham commented May 31, 2016 • edited

highfive commented May 31, 2016

edunham commented May 31, 2016

This comment has been minimized.

larsbergstrom Jun 1, 2016

larsbergstrom commented Jun 1, 2016

jdm commented Jun 2, 2016

larsbergstrom commented Jun 3, 2016

This comment has been minimized.

aneeshusa Jun 3, 2016

This comment has been minimized.

aneeshusa Jun 3, 2016

This comment has been minimized.

aneeshusa Jun 3, 2016

This comment has been minimized.

aneeshusa Jun 3, 2016

This comment has been minimized.

edunham Jun 3, 2016

highfive commented Jun 3, 2016

edunham commented Jun 3, 2016

aneeshusa commented Jun 3, 2016

edunham commented Jun 6, 2016

larsbergstrom commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 6, 2016

bors-servo commented Jun 6, 2016

edunham commented May 16, 2016 •

edited by larsbergstrom

frewsxcv May 18, 2016 •

edited

edunham commented May 31, 2016 •

edited