Part of #13551 - Create packages under release/debug directories as appropriate #13845

leikahing · 2016-10-20T05:12:18Z

This change implements fixes for #13551 for Linux and MacOS targets.

/python/servo/package_commands.py was modified so that:

On MacOS, it creates all intermediate packaging directories like dmg, brew, and brew-tmp under target/(release|debug), rather than in target directly.
On MacOS, all packaging artifacts (.dmg, brew .tar.gz) are packaged under target/(release|debug), rather than in target directly.
On Linux, the resulting tar.gz Servo package is placed under target/(release|debug), rather than in target.
Also did some extra cleanup around path parsing in the MacOS packaging code, to use os.path methods rather than straight '/' parsing with split and join where it was applicable.

/etc/ci/upload_nightly.sh was modified to:

Look for artifacts in target/release for mac, macbrew, and linux platforms, rather than just target/.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13551
These changes do not require tests because it is based on CI and packaging tools. They were manually tested for correctness.

…lease/debug as specified or detected. Modify macos packaging to create all packages under release/debug directory. Updated etc/ci/upload_nightly.sh to support uploading from either release/debug directory, depending on what was built

This change is

aneeshusa · 2016-10-20T12:32:22Z

I've only skimmed the description so far, but thanks for the PR, and extra thanks for going above and beyond! Cleaning up the path manipulations in the OS X builders has been on my TODO list for a long time :)

KiChjang · 2016-10-24T17:42:45Z

r? @aneeshusa

Highfive is slacking off it seems.

leikahing · 2016-10-24T19:29:04Z

Oh huh, thanks @KiChjang - I thought my original message assigned successfully.

aneeshusa

Looks good overall. I'll take a closer look at the path manipulation changes once they're split up. It looks like you're going to earn a lot of bonus points on this PR!

aneeshusa · 2016-10-28T21:15:10Z

etc/ci/upload_nightly.sh

@@ -25,9 +25,20 @@ upload() {
    s3cmd cp "${package_upload_path}" "${nightly_upload_dir}/servo-latest.${3}"
 }

+package_path() {


We only are packaging + uploading release builds for nightlies (at least right now), so no need to support debug builds at the moment.

aneeshusa · 2016-10-28T21:15:10Z

etc/ci/upload_nightly.sh


 main() {
-    if [[ "${#}" != 1 ]]; then
+    if (( "${#}" != 1 )); then


Huh, I never knew about this, +1 from me!

aneeshusa · 2016-10-28T21:15:10Z

python/servo/package_commands.py

@@ -153,14 +153,14 @@ def package(self, release=False, dev=False, android=None, debug=False, debugger=
                return e.returncode
        elif is_macosx():

-            dir_to_build = '/'.join(binary_path.split('/')[:-1])
+            dir_to_build = path.dirname(binary_path)


Please split out all of the hacking string processing -> os.path function changes into a separate commit.

aneeshusa · 2016-10-28T21:15:10Z

python/servo/package_commands.py

-            dir_to_dmg = '/'.join(binary_path.split('/')[:-2]) + '/dmg'
-            dir_to_app = dir_to_dmg + '/Servo.app'
-            dir_to_resources = dir_to_app + '/Contents/Resources/'
+            dir_to_dmg = path.join(path.dirname(binary_path), 'dmg')


First arg here is dir_to_build.

aneeshusa · 2016-10-28T21:15:10Z

python/servo/package_commands.py

-            dir_to_resources = dir_to_app + '/Contents/Resources/'
+            dir_to_dmg = path.join(path.dirname(binary_path), 'dmg')
+            dir_to_app = path.join(dir_to_dmg, 'Servo.app')
+            dir_to_resources = path.join(dir_to_app, 'Contents/Resources/')


path.join(dir_to_app, 'Contents', 'Resources')

aneeshusa · 2016-10-28T21:15:10Z

python/servo/package_commands.py

            time = now.replace(microsecond=0).isoformat()
            time = time.replace(':', '-')
-            dmg_path += time + "-servo-tech-demo.dmg"
+            dmg_path = path.join(dir_to_build, time + "-servo-tech-demo.dmg")


If it fits the line limit, I'd prefer string formatting for the second arg:
'{}-servo-tech-demo.dmg'.format(time)

aneeshusa · 2016-10-28T21:15:10Z

python/servo/package_commands.py

@@ -318,7 +317,7 @@ def package(self, release=False, dev=False, android=None, debug=False, debugger=
            print("Creating tarball")
            time = datetime.utcnow().replace(microsecond=0).isoformat()
            time = time.replace(':', "-")
-            tar_path = path.join(self.get_target_dir(), time + '-servo-tech-demo.tar.gz')
+            tar_path = path.join(os.path.dirname(binary_path), time + '-servo-tech-demo.tar.gz')


Same comment about using string formatting here if possible.

aneeshusa

I really liked the path cleanups you had previously - sorry if I wasn't clear but I still want them in this PR, just as a separate commit to avoid this directory change from getting lost.

Also, for the brew package it looks like the upload script has the new location but the packaging code hasn't been changed (although I seem to remember it being there before).

aneeshusa · 2016-10-30T06:41:21Z

etc/ci/upload_nightly.sh

@@ -25,9 +25,8 @@ upload() {
    s3cmd cp "${package_upload_path}" "${nightly_upload_dir}/servo-latest.${3}"
 }

-


Keep this line to avoid diff noise and fit with the double newline between functions style.

This is resolved in the next commit (haven't done any squashing yet in order to separate out my fixes and improvements).

aneeshusa · 2016-10-30T06:41:21Z

python/servo/package_commands.py

@@ -158,7 +158,7 @@ def package(self, release=False, dev=False, android=None, debug=False, debugger=
            now = datetime.utcnow()

            print("Creating Servo.app")
-            dir_to_dmg = '/'.join(binary_path.split('/')[:-2]) + '/dmg'
+            dir_to_dmg = '/'.join(dir_to_build) + '/dmg'


I think this should be path.join(dir_to_build, 'dmg').

Yes, I have a second commit incoming that does all the path tidying - just testing it all to make sure it works again as a blob (and because servo currently isn't building on my Mac for some reason).

leikahing · 2016-10-31T01:10:43Z

@aneeshusa - I committed all my path changes again - sorry for the confusion. I had to redo my commits to make them separate and since I have to build on two different machines, I committed to my fork before re-adding all my path modifications back in (now with more cleanups).

I re-tested everything on Linux/macos with all these changes - everything looks good on my end. Please re-review when you can. 😄

bors-servo · 2016-11-04T10:06:09Z

☔ The latest upstream changes (presumably #14058) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-11-09T22:48:17Z

Review ping @aneeshusa.

aneeshusa · 2016-11-15T05:42:49Z

python/servo/package_commands.py

-            dir_to_build = '/'.join(binary_path.split('/')[:-1])
-            dir_to_root = '/'.join(binary_path.split('/')[:-3])
+            dir_to_build = path.dirname(binary_path)
+            dir_to_root = os.sep.join(binary_path.split(os.sep)[:-3])


You should be able to use self.get_top_dir() to get dir_to_root.

aneeshusa · 2016-11-15T05:44:18Z

python/servo/package_commands.py

-            os.makedirs(dir_to_app + '/Contents/MacOS/')
-            shutil.copy2(dir_to_build + '/servo', dir_to_app + '/Contents/MacOS/')
+            shutil.copytree(path.join(dir_to_root, 'resources'), dir_to_resources)
+            shutil.copytree(browserhtml_path, dir_to_resources + path.split(browserhtml_path)[1])


The + can be replaced by a path.join, and you can use path.basename instead of splitting and indexing.

aneeshusa · 2016-11-15T05:44:49Z

python/servo/package_commands.py

+
+            content_dir = path.join(dir_to_app, 'Contents', 'MacOS')
+            os.makedirs(content_dir)
+            shutil.copy2(path.join(dir_to_build, 'servo'), content_dir)


Use binary_path directly.

aneeshusa · 2016-11-15T05:47:48Z

python/servo/package_commands.py


            print("Finding dylibs and relinking")
-            copy_dependencies(dir_to_app + '/Contents/MacOS/servo', dir_to_app + '/Contents/MacOS/')
+            copy_dependencies(path.join(dir_to_app, 'Contents', 'MacOS', 'servo'), content_dir)


You can reuse content_dir here inside the path.join.

aneeshusa · 2016-11-15T05:48:29Z

python/servo/package_commands.py

            with open(template_path) as template_file:
                template = mako.template.Template(template_file.read())
                with open(credits_path, "w") as credits_file:
                    credits_file.write(template.render(version=version))
            delete(template_path)

            print("Writing run-servo")
-            bhtml_path = path.join('${0%/*}/../Resources', browserhtml_path.split('/')[-1], 'out', 'index.html')
-            runservo = os.open(dir_to_app + '/Contents/MacOS/run-servo', os.O_WRONLY | os.O_CREAT, int("0755", 8))
+            bhtml_path = path.join('${0%/*}/../Resources', browserhtml_path.split(os.sep)[-1], 'out', 'index.html')


Replace the .split with native path functions.

Also, see if you can get rid of the explicit /s in the first path.join in favor of multiple args.

aneeshusa · 2016-11-15T05:49:08Z

python/servo/package_commands.py

-            bhtml_path = path.join('${0%/*}/../Resources', browserhtml_path.split('/')[-1], 'out', 'index.html')
-            runservo = os.open(dir_to_app + '/Contents/MacOS/run-servo', os.O_WRONLY | os.O_CREAT, int("0755", 8))
+            bhtml_path = path.join('${0%/*}/../Resources', browserhtml_path.split(os.sep)[-1], 'out', 'index.html')
+            runservo = os.open(path.join(dir_to_app, 'Contents', 'MacOS', 'run-servo'),


Reuse contents_dir.

leikahing · 2016-11-17T22:02:33Z

@aneeshusa Thanks again for feedback - I've cleaned up the code, squashed my commits into the 'feature change' commit and an enhancement commit, and changed my commit messages to conform to servo standards.

leikahing · 2016-12-07T21:45:49Z

Review ping @aneeshusa

aneeshusa · 2016-12-08T01:36:08Z

Sorry about the delay. You've got all the rights bits in terms of code and commit messages, but they need to be shuffled around a bit to match.

Switch the two commits so the first commit does all the cleanups.
This will make it easier to read the other commit, which changes the actual locations.
There are three changes that you need to migrate from 7a56f84 to f03cf09,
to completely separate cleanups from semantic changes. Each of these is a change of the path some variable refers to, and all of these are in the macOS packager:
- dmg_path, which is on line 217
- dir_to_brew, which is on line 229
- dir_to_tar, which is on line 230

leikahing · 2016-12-09T18:48:04Z

@aneeshusa Sorry for how long it's taken to get this PR in.

I've reordered my commits so that the path manipulation cleanups are the first commit.

I imagine your statement here:

There are three changes that you need to migrate from 7a56f84 to f03cf09

Was for the other direction (some changes in the path-manipulation change belonged in the 'change release packaging' commit), so the path manipulation commit now changes the os.path usage based on the original version of package_commands.py, and then the second commit to change the packaging structure improves upon that commit.

aneeshusa

Looks great! One last style nit.

aneeshusa · 2016-12-09T19:32:51Z

python/servo/package_commands.py

-            bhtml_path = path.join('${0%/*}/../Resources', browserhtml_path.split('/')[-1], 'out', 'index.html')
-            runservo = os.open(dir_to_app + '/Contents/MacOS/run-servo', os.O_WRONLY | os.O_CREAT, int("0755", 8))
+            bhtml_path = path.join('${0%/*}', '..', 'Resources', path.basename(browserhtml_path), 'out', 'index.html')
+            runservo = os.open(path.join(content_dir, 'run-servo'),


nit: use block indentation:

runservo = os.open( path.join(...), os.O_WRONLY..., int("0755", 8) )


        Cleanup path manipulations under macOS/Linux

Path manipulation has been changed from using '/' to using os.path functions for packaging code for macOS and Linux targets.


        Package macos/Linux builds under release/debug dirs

This commit updates the 'mach package' command to generate output under the target/release and target/debug directories when run on macOS and Linux. etc/ci/upload_nightly.sh has also been updated to upload packages from the release/debug directories.

aneeshusa · 2016-12-09T21:22:24Z

Everything looks good to me, thanks again for the PR and cleaning up all that path code! @bors-servo r+

bors-servo · 2016-12-09T21:22:27Z

📌 Commit 6a7fb6c has been approved by aneeshusa

bors-servo · 2016-12-09T22:46:15Z

⌛ Testing commit 6a7fb6c with merge 4cb3404...


        Auto merge of #13845 - birryree:package-under-target-profile-dirs, r=…

@aneeshusa

…aneeshusa Part of #13551 - Create packages under release/debug directories as appropriate r? @aneeshusa This change implements fixes for #13551 for Linux and MacOS targets. `/python/servo/package_commands.py` was modified so that: - On MacOS, it creates all intermediate packaging directories like `dmg`, `brew`, and `brew-tmp` under `target/(release|debug)`, rather than in `target` directly. - On MacOS, all packaging artifacts (`.dmg`, brew `.tar.gz`) are packaged under `target/(release|debug)`, rather than in `target` directly. - On Linux, the resulting `tar.gz` Servo package is placed under `target/(release|debug)`, rather than in `target`. - Also did some extra cleanup around path parsing in the MacOS packaging code, to use `os.path` methods rather than straight `'/'` parsing with `split` and `join` where it was applicable. `/etc/ci/upload_nightly.sh` was modified to: - Look for artifacts in `target/release` for `mac`, `macbrew`, and `linux` platforms, rather than just `target/`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13551 - [x] These changes do not require tests because it is based on CI and packaging tools. They were manually tested for correctness.  …lease/debug as specified or detected. Modify macos packaging to create all packages under release/debug directory. Updated etc/ci/upload_nightly.sh to support uploading from either release/debug directory, depending on what was built  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13845)

bors-servo · 2016-12-10T00:17:50Z

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

highfive assigned aneeshusa Oct 24, 2016

jdm added the S-awaiting-review label Oct 24, 2016

aneeshusa requested changes Oct 28, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 28, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from 3bec7c1 to 0942ae4 Oct 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 30, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from 0942ae4 to 3520247 Oct 30, 2016

aneeshusa requested changes Oct 30, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 30, 2016

highfive added the S-needs-rebase label Nov 4, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch 2 times, most recently from 7b01cbd to bc1283a Nov 4, 2016

jdm removed the S-needs-rebase label Nov 9, 2016

aneeshusa reviewed Nov 15, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Nov 15, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from bc1283a to eef6001 Nov 17, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 17, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from eef6001 to f03cf09 Nov 17, 2016

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Dec 8, 2016

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from f03cf09 to 800a181 Dec 9, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 9, 2016

aneeshusa requested changes Dec 9, 2016

View changes

leikahing added 2 commits Oct 30, 2016

Cleanup path manipulations under macOS/Linux

d4e9860

Path manipulation has been changed from using '/' to using os.path functions for packaging code for macOS and Linux targets.

leikahing force-pushed the leikahing:package-under-target-profile-dirs branch from 800a181 to 6a7fb6c Dec 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 9, 2016

aneeshusa approved these changes Dec 9, 2016

View changes

bors-servo merged commit 6a7fb6c into servo:master Dec 10, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 10, 2016

leikahing deleted the leikahing:package-under-target-profile-dirs branch Dec 10, 2016

servo / servo

Part of #13551 - Create packages under release/debug directories as appropriate #13845

Part of #13551 - Create packages under release/debug directories as appropriate #13845

leikahing commented Oct 20, 2016 •

edited

aneeshusa commented Oct 20, 2016

KiChjang commented Oct 24, 2016

leikahing commented Oct 24, 2016

aneeshusa left a comment

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.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

leikahing commented Oct 31, 2016

bors-servo commented Nov 4, 2016

jdm commented Nov 9, 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.

This comment has been minimized.

This comment has been minimized.

leikahing commented Nov 17, 2016

leikahing commented Dec 7, 2016

aneeshusa commented Dec 8, 2016

leikahing commented Dec 9, 2016 •

edited

aneeshusa left a comment

This comment has been minimized.

aneeshusa commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 10, 2016

		@@ -25,9 +25,8 @@ upload() {
		s3cmd cp "${package_upload_path}" "${nightly_upload_dir}/servo-latest.${3}"
		}

servo / servo

Join GitHub today

Part of #13551 - Create packages under release/debug directories as appropriate #13845

Part of #13551 - Create packages under release/debug directories as appropriate #13845

Conversation

leikahing commented Oct 20, 2016 • edited

aneeshusa commented Oct 20, 2016

KiChjang commented Oct 24, 2016

leikahing commented Oct 24, 2016

aneeshusa left a comment

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.

This comment has been minimized.

This comment has been minimized.

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

leikahing commented Oct 31, 2016

bors-servo commented Nov 4, 2016

jdm commented Nov 9, 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.

This comment has been minimized.

This comment has been minimized.

leikahing commented Nov 17, 2016

leikahing commented Dec 7, 2016

aneeshusa commented Dec 8, 2016

leikahing commented Dec 9, 2016 • edited

aneeshusa left a comment

This comment has been minimized.

aneeshusa commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 10, 2016

leikahing commented Oct 20, 2016 •

edited

leikahing commented Dec 9, 2016 •

edited