Closed Bug 1198179 Opened 4 years ago Closed 4 years ago
Cluster build-linux .sh artifact support broken
1.34 KB, text/plain
1.34 KB, text/plain
14.14 KB, patch
|Details | Diff | Splinter Review|
8.98 KB, patch
|Details | Diff | Splinter Review|
40 bytes, text/x-review-board-request
My cross-mac build in TC was almost successful, but failed copying build artifacts out: ``` for file in $DIST_TARGET_UPLOADS do mv *.$file $HOME/artifacts/target.$file done ++ for file in '$DIST_TARGET_UPLOADS' ++ mv firefox-43.0a1.en-US.mac64.dmg /home/worker/artifacts/target.mac64.dmg ++ for file in '$DIST_TARGET_UPLOADS' ++ mv firefox-43.0a1.en-US.mac64.json /home/worker/artifacts/target.mac64.json ++ for file in '$DIST_TARGET_UPLOADS' ++ mv firefox-43.0a1.en-US.mac64.common.tests.zip firefox-43.0a1.en-US.mac64.cppunittest.tests.zip firefox-43.0a1.en-US.mac64.mochitest.tests.zip firefox-43.0a1.en-US.mac64.reftest.tests.zip firefox-43.0a1.en-US.mac64.web-platform.tests.zip firefox-43.0a1.en-US.mac64.xpcshell.tests.zip /home/worker/artifacts/target.tests.zip mv: target '/home/worker/artifacts/target.tests.zip' is not a directory ``` It's failing because bug 917999 split parts of tests.zip out into separate zip files, so having "tests.zip" in DIST_TARGET_UPLOADS is causing us to try to `mv *tests.zip $HOME/artifacts/target.tests.zip`, which is failing because *tests.zip expands to multiple files.
Morgan: you mentioned on IRC that you were looking into doing something better with uploads. Does that cover this?
I found this in my "utilities folder," I believe it's what I had written to solve this problem.
Unfortunately mshal literally just changed this last week in bug 1118778 so that the uploadFiles property is set from upload.py as part of `make upload`: https://dxr.mozilla.org/mozilla-central/source/build/upload.py#200 ...so this won't work because we're not calling `make upload` on TC. :-/ That being said, I think this is the right approach, we just need to wire up "have the build system emit info about what it wants to upload" in some other fashion. We could tweak `make upload` to have an option to simply upload files to a local directory, and set that for taskcluster builds so that the build system would do the work for us here. The only fiddly bit is the renaming we're currently doing, we'll still have to sort something out there.
I was already fiddling with upload.py. This is somewhat of a precursor patch that just kills gen_mach_buildprops.py and does all the properties stuff in upload.py
Attachment #8652911 - Flags: review?(ted)
This reworks upload.py to always spit out a properties file, even if there are no UPLOAD_HOST/UPLOAD_USER environment settings. In this case, you'd just get the packageFilename and uploadFiles properties. So TC builds should be able to remove the UPLOAD_* environment settings and still get the properties out. Does that seem feasible?
Attachment #8652916 - Flags: review?(ted)
That sounds pretty good. It doesn't *completely* solve our problems with TC, in that TC wants to rename the build files to $target.whatever.ext so that the routes are easy to bake into the task descriptions (without version numbers). I don't think it'd be hard for the build system to stick that extra info into the properties file, though. Maybe an additional annotation on files specifying what route they should have?
Yeah, it seems like upload-files.mk has most of the information we need here. It already has variables for PACKAGE, INSTALLER_PACKAGE, COMPLETE_MAR, etc. I think we'd probably need a better way to export that info either to upload.py or directly into some other properties file. Right now only $(PACKAGE) is passed into upload.py to get the packageFilename property.
Comment on attachment 8652911 [details] [diff] [review] 0001-Bug-1198179-Kill-gen_mach_buildprops.py.patch Review of attachment 8652911 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, nitpicked the Python code a bit before realizing you were copying it from gen_mach_buildprops.py. ::: build/upload.py @@ +127,5 @@ > + > + try: > + # open in binary mode to make sure we get consistent results > + # across all platforms > + f = open(filename, "rb") We generally use `with open(filename, "rb") as f:` @@ +133,5 @@ > + sha512Hash = shaObj.hexdigest() > + > + size = os.path.getsize(filename) > + except: > + pass It'd be good to at least log the exception here, unless you expect us to hit this during every execution for some reason. Actually, you're already checking for file existence in GetMarProperties so maybe this should be fatal?
Attachment #8652911 - Flags: review?(ted) → review+
Comment on attachment 8652916 [details] [diff] [review] 0002-Bug-1198179-make-upload.py-write-properties-even-if-.patch Review of attachment 8652916 [details] [diff] [review]: ----------------------------------------------------------------- This is good, but I'd like to take it one step further and give upload.py the ability to copy files to a local directory, so Taskcluster builds could simply set a variable and have the build artifacts wind up where it wants them. The build system knows all the files it wants to upload, so it makes sense to do things there.
Attachment #8652916 - Flags: review?(ted) → review+
Attachment #8652391 - Attachment mime type: text/x-python-script → text/plain
Summary: TaskCluster build-linux.sh broken trying to copy split test zips → TaskCluster build-linux.sh artifact support broken
After much discussion in #releng, we've boiled this down into two phases, with the first better-defined than the second. There are three modes we need to support: 1. BB-based build jobs, BB-baseed test jobs (current configuration) 2. TC-based build jobs, BB-based test jobs (via BBB) 3. TC-based build jobs, TC-based test jobs Greg has pointed out that the typical way to link build and test jobs in TC is for the build job to have a static description of its outputs in the task definition itself. The task-graph creation code then uses this information to create appropriate artifact URLs which it substitutes into the test task definitions. We'd like to use the same approach for consistency. So here's the plan: In TC jobs, re-enable MOZ_AUTOMATION_UPLOAD, and add some env-based config that "stubs out" the upload to just copy files to UPLOAD_PATH(=~/artifacts) locally, and doesn't generate properties at all. Create simple package names (bug 1203573), and specify those names in the build task definition. Next, we'll write task definitions for tests that stuff the artifact URLs into Buildbot properties (via BBB). When it's time to start running test jobs in TC (mode 3), we still have the URLs we need available to the new task definitions.
Since Ehsan asked -- I'm hoping to get this patched today, and probably landed on Monday. But I'll be at a work-week next week, so if things get "complicated" it'll likely take until the week after.
Here's where I'm at: I've modified upload.py to special case UPLOAD_HOST=localhost to just copy files to UPLOAD_PATH. In this case, there's no post_upload_command and no properties generated. I removed MOZ_AUTOMATION_UPLOAD=0 so `make upload` will run, and added MOZ_SIMPLE_PACKAGE_NAME=target (although I haven't checked whether your patch is an ancestor of this). https://tools.taskcluster.net/task-inspector/#JJxRA2iPRpqVA60VsxHpQw/0 https://hg.mozilla.org/users/dmitchell_mozilla.com/mozilla-central/rev/7af2cb9be8c3 I think all the ducks are lined up here, and it's just squashing stupid typos after long try runs. I'll be at a work-week for the next week, so reassigning to Ted in hopes he can put this over the finish line (or r+ and land if that try job goes green).
Assignee: dustin → ted
Indentation error caused a failure, but the uploads ran. Fixed: https://tools.taskcluster.net/task-inspector/#JJxRA2iPRpqVA60VsxHpQw/0 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c442b8efe9d .. that worked, but failed doing the *old* DIST_* stuff I forgot to remove
I figure this next run will succeed, so I'm marking it for review. If it does succeed and passes review, please go ahead and land it. https://tools.taskcluster.net/task-inspector/#QBNCUEc1TiCjyfBfyKnR2A/0
Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal
GREEN BUILD! :)
Comment on attachment 8660370 [details] MozReview Request: Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal https://reviewboard.mozilla.org/r/19121/#review17009 This looks good, we just need to change the Mac task definition to match. ::: build/upload.py:20 (Diff revision 1) > +# properties are written out. We decided that BBB was going to be responsible for generating the properties file from the Task data, correct? ::: build/upload.py:256 (Diff revision 1) > + remote_path = GetRemotePath(path, file, base_path) Feels weird to be using `GetRemotePath` to construct a local path, even if it's doing the right thing. Can you at least change the comment to not say "remotely"? ::: testing/taskcluster/scripts/builder/build-linux.sh (Diff revision 1) > -done All the removals from this script are great! ::: testing/taskcluster/tasks/builds/linux64_clobber.yml:50 (Diff revision 1) > - build: 'public/build/target.linux-x86_64.tar.bz2' > + build: 'public/build/target.tar.bz2' We need to make this same change in the Mac task definitions. We're sure nothing is depending on the existing naming, right? ::: testing/taskcluster/tasks/builds/linux64_clobber.yml:51 (Diff revision 1) > tests: 'public/build/target.tests.zip' We could probably pull locations:tests up into the base build.yml, unless our yaml inheritance doesn't let you append to lists. ::: testing/taskcluster/tasks/builds/opt_macosx64.yml:59 (Diff revision 1) > build: 'public/build/target.mac64.dmg' This needs to be changed, as per above.
Attachment #8660370 - Flags: review+
Comment on attachment 8660370 [details] MozReview Request: Bug 1198179: in taskcluster, have 'make upload' just copy; r?mshal ted got to it first :)
I've updated the "remote path" bits (just renaming). I didn't update the mac builds, but that should be straightforward. Since I don't trust myself: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0526ce5cc6f If that's green, ship it.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.