Closed Bug 514466 Opened 12 years ago Closed 12 years ago

uploadsymbols target fails when for certain filenames

Categories

(Firefox Build System :: General, defect, P2)

defect

Tracking

(status1.9.2 beta1-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: bhearsum, Assigned: coop)

References

Details

Attachments

(3 files)

Upload symbols has two problems with files with more than one space in name. Firstly, the hash calculation, http://hg.mozilla.org/mozilla-central/file/tip/toolkit/crashreporter/tools/upload_symbols.sh#l51, assumes the output looks like 
  SHA1(dist/namoroko-3.6a1-crashreporter-symbols.zip)=1210b518ae71241c853b7d9cf795f2e0ce90c855

and gets confused if there are spaces in the filename. Secondly, the quoting in the scp upload, http://hg.mozilla.org/mozilla-central/file/tip/toolkit/crashreporter/tools/upload_symbols.sh#l54 doesn't cope with multiple spaces. Mysteriously, it does cope with single spaces, eg Firefox 3.5.2.crashreporter-symbols.zip.

This needs to be fixed before 3.6b1
For the hash we could just switch to using the PID of the make process. That should be sufficiently unique to avoid the simultaneous upload by two hosts, which occurs only infrequently.

Don't think we hit this with 3.5b1, did something change to break 3.6b1 ? The switch to official branding may mean we're OK until 3.7a1.
(In reply to comment #1)
> For the hash we could just switch to using the PID of the make process. That
> should be sufficiently unique to avoid the simultaneous upload by two hosts,
> which occurs only infrequently.
> 
> Don't think we hit this with 3.5b1, did something change to break 3.6b1 ? The
> switch to official branding may mean we're OK until 3.7a1.

J'accuse bug 504953
Blocks: 512300
Assignee: nobody → ccooper
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
For the new hash parsing, I simply throw away everything before the equal sign and then remove any extraneous spaces. Assigning via backticks seems to remove those spaces automatically, but I want to be sure.

AFAICT, the accepted way to transfer files with spaces is to escape the spaces with backslashes and then double-quote the entire target path.
Attachment #401076 - Flags: review?(bhearsum)
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

looks good
Attachment #401076 - Flags: review?(bhearsum) → review+
Component: Release Engineering → Build Config
Product: mozilla.org → Core
QA Contact: release → build-config
Version: other → Trunk
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

This is not shipping code, but does get run by the build system for every build that is created.

Checked in on m-c: http://hg.mozilla.org/mozilla-central/rev/b4ef29fbae96
Attachment #401076 - Flags: approval1.9.2?
Attachment #401076 - Flags: approval1.9.1.4?
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

FYI, we only need the archive space-escape changes on 1.9.1. We don't append the hash on that branch, and there's really no point in adding it now.
Attachment #401076 - Flags: approval1.9.1.4? → approval1.9.1.4+
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

Approved for 1.9.1.4, a=dveditz
Attachment #401076 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

Further testing in staging reveals that we don't need this on 1.9.1 at all. Sorry for the trouble, dveditz.
Attachment #401076 - Flags: approval1.9.1.4+
Comment on attachment 401076 [details] [diff] [review]
[checked in] Fix hash parsing, escape spaces prior to upload

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/642dcbef2f4a
Attachment #401076 - Attachment description: Fix hash parsing, escape spaces prior to upload → [checked in] Fix hash parsing, escape spaces prior to upload
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
When doing a test 3.6b1 release, we get symbols uploaded but then fail to unpack the archive:

make uploadsymbols
/bin/sh /builds/slave/macosx_build/build/toolkit/crashreporter/tools/upload_symbols.sh "./dist/Firefox 3.6 Beta 1.crashreporter-symbols.zip"
Transferring symbols... ./dist/Firefox 3.6 Beta 1.crashreporter-symbols.zip

Firefox 3.6 Beta 1.crashreporter-symbols.zip    0%    0     0.0KB/s   --:-- ETA
[snip]
Firefox 3.6 Beta 1.crashreporter-symbols.zip  100%  199MB  11.0MB/s   00:18    
Unpacking symbols on remote host...
unzip:  cannot find or open 2ddef3e494fa711081717ee323a6c8dbac14640c-Firefox\ 3.6\ Beta\ 1.crashreporter-symbols.zip, 2ddef3e494fa711081717ee323a6c8dbac14640c-Firefox\ 3.6\ Beta\ 1.crashreporter-symbols.zip.zip or 2ddef3e494fa711081717ee323a6c8dbac14640c-Firefox\ 3.6\ Beta\ 1.crashreporter-symbols.zip.ZIP.
make: *** [uploadsymbols] Error 9

I think we should just drop the spaces in the filename, which only occur in release builds where we don't preserve the file anyway (just its contents!).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This makes MOZ_PKG_PRETTYNAMES irrelevant to the symbol and packaged-test archive naming. It resolves the issue I was seeing on mac by creating firefox-3.6b1.crashreporter-symbols.zip. The file hash prevents collisions should more than one platform upload at the same time. 

Think I have to ask review from Ted on this, destined for 1.9.2 ahead of 3.6b1.
Attachment #404209 - Flags: review?(ted.mielczarek)
Comment on attachment 404209 [details] [diff] [review]
Use a plain name, doesn't have to be end-user friendly

This won't work. We need the symbol and test packages to have the same basename as the build for the builders that run packaged tests (and bug 457753 will run those on releases).

Incidentally, I filed bug 519679 yesterday about probably this same issue because catlee hit it. I suggested there that we just override the destination filename ('archive' in the upload-symbols script), since that in fact doesn't matter.
Attachment #404209 - Flags: review?(ted.mielczarek) → review-
Duplicate of this bug: 519679
Ted's suggestion from bug 519679, using the sed foo that coop already wrote. This works on the big-three platforms on a fake 3.6b1 release.
Attachment #404574 - Flags: review?(ted.mielczarek)
Attachment #404574 - Flags: review?(ted.mielczarek)
Attachment #404574 - Flags: review+
Attachment #404574 - Flags: approval1.9.2+
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.