Closed
Bug 1243750
Opened 9 years ago
Closed 9 years ago
mac sdk generated in the wrong place
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox45 fixed, firefox46 fixed, firefox47 fixed)
RESOLVED
FIXED
mozilla47
People
(Reporter: bhearsum, Assigned: mshal)
References
Details
(Keywords: qablocker)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
glandium
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
It ends up in places like http://ftp.mozilla.org/pub/firefox/releases/44.0/Firefox-44.0.en-US.mac-x86_64.sdk.tar.bz2 instead of http://ftp.mozilla.org/pub/firefox/releases/44.0/mac/en-US.
The capital letters make me suspicious that something may be wrong or inconistent in package-name/packager/upload.mk.
Assignee | ||
Comment 1•9 years ago
|
||
I think the location is different because of this OSX-specific code in upload-files.mk:
SDK = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).sdk$(SDK_SUFFIX)
ifeq ($(MOZ_APP_NAME),xulrunner)
SDK = $(SDK_PATH)$(MOZ_APP_NAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).sdk$(SDK_SUFFIX)
endif
Specifically, since MOZ_APP_NAME is no longer 'xulrunner', we use the first SDK definition, which doesn't have $(SDK_PATH). SDK_PATH is defined as:
SDK_PATH = $(PKG_PATH)
ifeq ($(MOZ_APP_NAME),xulrunner)
SDK_PATH = sdk/
endif
So the SDK ends up in the same place as the package for Linux & Windows, but since the OSX specific code ignores SDK_PATH when we're not building xulrunner, it gets dumped into the top-level.
Do we want to change this structure so that the SDK is always built in "xulrunner-mode", meaning SDK_PATH = sdk, and we use the second 'SDK' definition for OSX? This would put the sdk in a separate 'sdk' directory, not alongside the package. If we don't want to move the sdks there, we at least need to add $(SDK_PATH) to this line I think:
SDK = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).sdk$(SDK_SUFFIX)
Does any code inside an 'if MOZ_APP_NAME == xulrunner' block do anything anymore? There's a bunch, not just in Makefiles, but also some test .js files.
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Michael Shal [:mshal] from comment #1)
> I think the location is different because of this OSX-specific code in
> upload-files.mk:
>
> SDK =
> $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).
> sdk$(SDK_SUFFIX)
> ifeq ($(MOZ_APP_NAME),xulrunner)
> SDK =
> $(SDK_PATH)$(MOZ_APP_NAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).
> sdk$(SDK_SUFFIX)
> endif
>
> Specifically, since MOZ_APP_NAME is no longer 'xulrunner', we use the first
> SDK definition, which doesn't have $(SDK_PATH). SDK_PATH is defined as:
>
> SDK_PATH = $(PKG_PATH)
> ifeq ($(MOZ_APP_NAME),xulrunner)
> SDK_PATH = sdk/
> endif
>
> So the SDK ends up in the same place as the package for Linux & Windows, but
> since the OSX specific code ignores SDK_PATH when we're not building
> xulrunner, it gets dumped into the top-level.
>
> Do we want to change this structure so that the SDK is always built in
> "xulrunner-mode", meaning SDK_PATH = sdk, and we use the second 'SDK'
> definition for OSX? This would put the sdk in a separate 'sdk' directory,
> not alongside the package. If we don't want to move the sdks there, we at
> least need to add $(SDK_PATH) to this line I think:
I think it makes sense to put them in "sdks" if it's easy to. I'm worried there's still more twists and turns in this rabbit hole though :(.
> SDK =
> $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).
> sdk$(SDK_SUFFIX)
>
> Does any code inside an 'if MOZ_APP_NAME == xulrunner' block do anything
> anymore? There's a bunch, not just in Makefiles, but also some test .js
> files.
I don't think so. XULRunner is a dead product...
Assignee | ||
Comment 3•9 years ago
|
||
This does a few things:
1) Add $(SDK_PATH) to the SDK definition for OSX, so it doesn't end up in the top-level directory
2) Force SDK_PATH to always be 'sdk', which is essentially the xulrunner behavior. This puts the sdks into dist/sdk locally, and should put them into an sdk subdirectory with post_upload.py, I think. :nthomas, is this correct? The try behavior seems different so it's a little hard to verify.
3) Removes some dead xulrunner code in package-name.mk
Assignee: nobody → mshal
Attachment #8713875 -
Flags: review?(mh+mozilla)
Attachment #8713875 -
Flags: feedback?(nthomas)
Comment 4•9 years ago
|
||
Comment on attachment 8713875 [details] [diff] [review]
0001-Bug-1243750-Install-all-SDKs-into-sdk.patch
(In reply to Michael Shal [:mshal] from comment #3)
> 2) Force SDK_PATH to always be 'sdk', which is essentially the xulrunner
> behavior. This puts the sdks into dist/sdk locally, and should put them into
> an sdk subdirectory with post_upload.py, I think. :nthomas, is this correct?
> The try behavior seems different so it's a little hard to verify.
I'm not sure tbh. My guess is that the --release-to-candidates case will work, but what is the desired behaviour for dep builds ? Dirs like http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-macosx64/1454263346/ don't appear to have a sdk. I bring this up because release-promotion world will have dep and then use beetmover to rename+move files.
The code is at https://github.com/mozilla-services/product-delivery-tools/tree/master/post_upload if you want to read some Go. Alternatively you can test in the staging setup by modifying some environment variables
UPLOAD_HOST="upload.ffxbld.productdelivery.stage.mozaws.net"
and POST_UPLOAD_CMD ending with
--bucket-prefix net-mozaws-stage-delivery
Or just ssh to ffxbld@upload.ffxbld.productdelivery.stage.mozaws.net from any slave, set up some test files in /tmp/<tmpdir> and call post_upload directly.
Check the output and/or http://bucketlister-delivery.stage.mozaws.net/ for results.
Attachment #8713875 -
Flags: feedback?(nthomas)
Comment 5•9 years ago
|
||
So if we are going to put those builds into a separate directory (thanks a lot for doing that!!) do we have a chance to get this also fixed for the 44.0 release? If yes, I wouldn't necessarily have to update mozdownload but if not I would expect breakage for our update tests once the next release is out and we have to test updates from a 44.0 release build.
Comment 6•9 years ago
|
||
Comment on attachment 8713875 [details] [diff] [review]
0001-Bug-1243750-Install-all-SDKs-into-sdk.patch
Review of attachment 8713875 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/mozapps/installer/upload-files.mk
@@ +535,4 @@
> # The plst and blkx resources are skipped because they belong to each
> # individual dmg and are created by hdiutil.
> SDK_SUFFIX = .tar.bz2
> +SDK = $(SDK_PATH)$(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).mac-$(TARGET_CPU).sdk$(SDK_SUFFIX)
SDK is already set to $(SDK_PATH)$(PKG_BASENAME).sdk$(SDK_SUFFIX) and $(SDK_PATH)$(PKG_BASENAME)-$(TARGET_CPU).sdk$(SDK_SUFFIX) on universal builds. PKG_BASENAME being $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).$(MOZ_PKG_PLATFORM), and MOZ_PKG_PLATFORM being mac on universal builds, this means you shouldn't even need this assignment.
Attachment #8713875 -
Flags: review?(mh+mozilla) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
This removes the separate OSX assignment. However, on OSX the PKG_BASENAME is actually '$(MOZ_PKG_APPNAME) $(MOZ_PKG_LONGVERSION)', which contains a space. So we have to change a few things where SDK and $(SDK).asc are used to account for spaces in the filename.
I also changed the SDK_PATH to be $(PKG_PATH)/sdk/ instead of just sdk/, otherwise the SDKs for all platforms go into the same directory, rather than a per-platform directory.
Since this causes OSX builds to create the SDK in $(SDK_PATH), it also fixes uploading the i386 version since that's where the wildcards in the UNIFY_DIST block are looking. However, the i386 version still goes into the top-level build directory due to how full paths are handled in post_upload, I believe (see also https://bugzilla.mozilla.org/show_bug.cgi?id=1245272#c2 ). If post_upload.py can't be changed to account for this, we may need to create the i386 sdk in objdir/x86_64/dist/mac/en-US/sdk so that it ends up in the right place.
Attachment #8713875 -
Attachment is obsolete: true
Attachment #8716493 -
Flags: review?(mh+mozilla)
Updated•9 years ago
|
Attachment #8716493 -
Flags: review?(mh+mozilla) → review+
Comment 9•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment 10•9 years ago
|
||
sorry had to back this out seems this caused problems on OSX and Linux Nightly Builds like https://treeherder.mozilla.org/logviewer.html#?job_id=3268145&repo=mozilla-central
Status: RESOLVED → REOPENED
Flags: needinfo?(mshal)
Resolution: FIXED → ---
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
I didn't mean to create this attachment or modify this bug--accidentally used `hg bzexport` incorrectly. Could someone help me undo the changes?
Assignee | ||
Updated•9 years ago
|
Assignee: pineapple.rice → mshal
Flags: needinfo?(mshal)
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Here's my latest attempt. I moved SDK_PATH into package-name.mk, so it is empty when PKG_PATH is empty for nightlies (which is what caused the bustage).
I also changed build/upload.py to cause the i386 sdk to be uploaded in the 'mac/en-US' directory for release builds. I'm still testing this out in staging, but it seemed to work when I ran things manually. Basically we just need to strip off the '.../obj-firefox/i386/dist' path similar to how we strip off '.../obj-firefox/x86_64/dist'. When the paths don't match, the files end up in the top-level directory of the release. post_upload.py just copies the files as they are in the temporary directory, so trying to fix it there won't help.
Attachment #8716493 -
Attachment is obsolete: true
Attachment #8717970 -
Attachment is obsolete: true
Attachment #8718105 -
Attachment is obsolete: true
Attachment #8718114 -
Attachment is obsolete: true
Attachment #8718565 -
Flags: review?(mh+mozilla)
Updated•9 years ago
|
Attachment #8718565 -
Flags: review?(mh+mozilla) → review+
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8718565 [details] [diff] [review]
0001-Bug-1243750-Install-all-SDKs-into-sdk-r-glandium.patch
I believe we just need to uplift this patch to aurora/beta/release in order for the SDK to be built and uploaded to the correct release directory. I've tried to test it as best I can in staging, but that's always a bit of a wildcard. It's currently running in nightly, though nightly and release builds differ in how this logic is invoked, so that isn't the best indicator.
Approval Request Comment
[Feature/regressing bug #]: 1233005, 1243750, 1245272
[User impact if declined]: SDKs for OSX will not be available for the release
[Describe test coverage new/current, TreeHerder]: Patch is running on nightly, and has been tested in staging.
[Risks and why]: Staging test environment is not really the same as production.
[String/UUID change made/needed]:
Attachment #8718565 -
Flags: approval-mozilla-release?
Attachment #8718565 -
Flags: approval-mozilla-beta?
Attachment #8718565 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox45:
--- → affected
status-firefox46:
--- → affected
Comment 20•9 years ago
|
||
Comment on attachment 8718565 [details] [diff] [review]
0001-Bug-1243750-Install-all-SDKs-into-sdk-r-glandium.patch
Important regression, taking it.
Should be in 45 beta 9
Attachment #8718565 -
Flags: approval-mozilla-beta?
Attachment #8718565 -
Flags: approval-mozilla-beta+
Attachment #8718565 -
Flags: approval-mozilla-aurora?
Attachment #8718565 -
Flags: approval-mozilla-aurora+
Comment 21•9 years ago
|
||
bugherder uplift |
Comment 22•9 years ago
|
||
bugherder uplift |
Comment 23•9 years ago
|
||
Comment on attachment 8718565 [details] [diff] [review]
0001-Bug-1243750-Install-all-SDKs-into-sdk-r-glandium.patch
No longer relevant
Attachment #8718565 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Comment 24•9 years ago
|
||
[bugday-20160323]
Status: RESOLVED,FIXED -> UNVERIFIED
Comments:
STR: Not clear.
Developer specific testing
Component:
Name Firefox
Version 46.0b9
Build ID 20160322075646
Update Channel beta
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS Windows 7 SP1 x86_64
Expected Results:
Developer specific testing
Actual Results:
As expected
Updated•6 years ago
|
Component: Build Config → General
Product: Firefox → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•