Current repack bustage: win32/en-US/seamonkey-2.48b1.zip c:/builds/slave/rel-c-b-rpk/build/comm-beta/objdir-l10n/_virtualenv/Scripts/python.exe c:/builds/slave/rel-c-b-rpk/build/comm-beta/mozilla/config/nsinstall.py -D c:/builds/slave/rel-c-b-rpk/build/comm-beta/objdir-l10n/dist/win32/en-US/ (cd c:/builds/slave/rel-c-b-rpk/build/comm-beta/objdir-l10n/dist/win32/en-US/ && \ wget --no-cache -nv --no-iri -N -O win32/en-US/seamonkey-2.48b1.zip 'http://archive.mozilla.org/pub/seamonkey/candidates/2.48b1-candidates/build2/unsigned/win32/en-US/seamonkey-2.48b1.zip') WARNING: timestamping does nothing in combination with -O. See the manual for details. win32/en-US/seamonkey-2.48b1.zip: No such file or directory c:/builds/slave/rel-c-b-rpk/build/comm-beta/mozilla/toolkit/locales/l10n.mk:196: recipe for target 'wget-en-US' failed pymake\..\..\mozmake.exe: *** [wget-en-US] Error 1 This stems from a change from bug 1301509. The warning isn't the problem. The problem is that it's downloading the seamonkey-2.48b1.zip to win32/en-US when it's already in <objdir>/dist/win32/en-US so my understanding is that PKG_PATH already includes win32/en-US. so since Firefox 51 wasn't complaining about this(or not that I noticed), there must be a change in the PKG_PATH that we need to set or something.

$(PACKAGE) == win32/en-US/seamonkey-2.48b1.zip $(PKG_PATH) == win32/en-US/ so given: wget-en-US: ifndef WGET $(error Wget not installed) endif $(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH) (cd $(ABS_DIST)/$(PKG_PATH) && \ $(WGET) --no-cache -nv --no-iri -N -O $(PACKAGE) '$(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME)') @echo 'Downloaded $(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME) to $(ABS_DIST)/$(PKG_PATH)/$(PACKAGE)' if it already goes into $(ABS_DIST)/$(PKG_PATH), i.e. dist/win32/en-US, and this is an empty directory, would "wget -O $(PACKAGE) ..." fail since it's trying to save the ZIP file into win32/en-US/win32/en-US? What change are we missing?

This was added in Bug 1293943 for Lightning. I think it needs to be backed out as it never worked right. TB doesn't have it.

Sorry forget the comment. wrong wget...

Both TB and Firefox don't have this problem because they seem to be using mozharness or release_repack.sh which does the magic. And with l10n dep repacks removed from Moco's buildbotcustom code (bug 1268895), this part of the code |make wget-en-US| isn't used. That said, there are probably other builders that use this rule.

This is a workaround. I'm not entirely sure changing the rule in toolkit/locales/l10n.mk is a good idea since I don't know if anything else uses that rule. That said, since it'll be pushed to a relbranch, I don't think it should matter; *BUT*, eventually will hit this issue *again* for the next release round so we need some sort of fix.

Given: wget-en-US: ifndef WGET $(error Wget not installed) endif $(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH) (cd $(ABS_DIST)/$(PKG_PATH) && \ $(WGET) --no-cache -nv --no-iri -N -O $(PACKAGE) '$(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME)') @echo 'Downloaded $(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME) to $(ABS_DIST)/$(PKG_PATH)/$(PACKAGE)' ifdef RETRIEVE_WINDOWS_INSTALLER ifeq ($(OS_ARCH), WINNT) $(NSINSTALL) -D $(ABS_DIST)/$(PKG_INST_PATH) (cd $(ABS_DIST)/$(PKG_INST_PATH) && \ $(WGET) --no-cache -nv --no-iri -N '$(EN_US_BINARY_URL)/$(PKG_PATH)$(PKG_INST_BASENAME).exe') @echo 'Downloaded $(EN_US_BINARY_URL)/$(PKG_PATH)$(PKG_INST_BASENAME).exe to $(ABS_DIST)/$(PKG_INST_PATH)$(PKG_INST_BASENAME).exe' endif endif if I set MOZ_PKG_PRETTYNAMES=1, both the wget for the zip and the installer fails. if I set MOZ_PKG_PRETTYNAMES= (ie. remove from env) the wget for the zip file succeeds but the installer fails because PKG_PATH is not set and thus $(EN_US_BINARY_URL)/$(PKG_PATH)$(PKG_INST_BASENAME).exe becomes: http://archive.mozilla.org/pub/seamonkey/candidates/2.48b1-candidates/build2/unsigned/seamonkey-2.48b1.en-US.win32.installer.exe which isn't right. The reason why Firefox and TB don't encounter this is because their EN_US_BINARY_URL points to <some taskcluster url>/build/<installer>.exe Since we use archive.mozilla.org directly AND our installer.exe is stored in <archive.mozilla.org url>/win32/en-US, this won't work. So would the workaround be something that we can consider? [tagging n-i from IanN, frg and Callek]

ewong your patch is rather empty :)

(In reply to Frank-Rainer Grahl from comment #9) > ewong your patch is rather empty :) as usual.. I forgot to do the qrefresh before attaching...

On second thoughts, while PrettyNames will go away in Gecko 54, I don't think it's a good idea to jump the gun with this change. So back to the workaround.

Came up with this idea and tried it on the loaner and it works.

Comment on attachment 8846213 [details] [diff] [review] proposed patch (option 2) Looks good. I would put a comment in with the bug reference or an explanation in there too and it might break something else later but if it works on the loaner hopefully not.

Well, it does break the final repack step, so going to go back to the drawing board.

(In reply to Edmund Wong (:ewong) from comment #14) > Well, it does break the final repack step, so going to go back to the > drawing board. by changing the PACKAGE variable, I've changed the ZIP_IN variable which is used in the make-installers-% stage. https://hg.mozilla.org/releases/mozilla-beta/file/tip/toolkit/locales/l10n.mk#l42

The very *worst* case scenario is to backout https://hg.mozilla.org/releases/mozilla-beta/35c4ed28dbac because doing that on the loaner fixes the problem. However, that said, backing out is just a temp-fix for our current release. It needs repeating for other releases or fixed permanently.

(In reply to Edmund Wong (:ewong) from comment #16) > The very *worst* case scenario is to backout > > https://hg.mozilla.org/releases/mozilla-beta/35c4ed28dbac That URL returns an error, it's https://hg.mozilla.org/releases/mozilla-beta/rev/35c4ed28dbac ;-) That said, I'm perplexed why that patch could even cause an error, are we overriding EN_US_PACKAGE_NAME? Also, if backing that out works, and before that one a plain $(PACKAGE) was used, why do we suddenly need a basename on it?

In any case, I'm happy about any workaround that works, at least for this release.

(In reply to Robert Kaiser from comment #17) > (In reply to Edmund Wong (:ewong) from comment #16) > > The very *worst* case scenario is to backout > > > > https://hg.mozilla.org/releases/mozilla-beta/35c4ed28dbac > > That URL returns an error, it's > https://hg.mozilla.org/releases/mozilla-beta/rev/35c4ed28dbac ;-) > > That said, I'm perplexed why that patch could even cause an error, are we > overriding EN_US_PACKAGE_NAME? > We aren't overriding EN_US_PACKAGE_NAME. > Also, if backing that out works, and before that one a plain $(PACKAGE) was > used, why do we suddenly need a basename on it? Given the new wget-en-US code as follows (first part): wget-en-US: ifndef WGET $(error Wget not installed) endif $(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH) (cd $(ABS_DIST)/$(PKG_PATH) && \ $(WGET) --no-cache -nv --no-iri -N -O $(PACKAGE) '$(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME)') @echo 'Downloaded $(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME) to $(ABS_DIST)/$(PKG_PATH)/$(PACKAGE)' Before, this patch, it would cd $(ABS_DIST)/$(PKG_PATH) and then wget the file as specified in $(EN_US_BINARY_URL)/$(PACKAGE) (which is win32/en-US/<pkgname>). However, now, it specifies -O $(PACKAGE). So the above is essentially: 1) create <dist>/win32/en-US 2) cd <dist>/win32/en-US 3) wget --no-cache -nv --no-iri -N -O win32/en-US/<pkgname> \ 'https://archive.mozilla.org/pub/seamonkey/candidates/2.48b1-candidates/build2/unsigned/win32/en-US/<pkgname> but since we're already *in* <dist/win32/en-US> which was just created in #1, nothing should be in it, but now we're expecting to save a win32/en-US/<pkgname> to this folder (as specified by -O $(PACKAGE)) and thusly it fails because wget doesn't create folders for us. Thus we need the BASEPACKAGE to get rid of the paths before <pkgname> And with TB and Firefox both: 1) don't do l10n dep builds 2) base all their EN_US_BINARY_URL on a taskcluster build path, they don't hit this. 1) cd $(ABS_DIST)/$(PKG_PATH) [ <dist>/win32/en-US ] 2) wget --no-cache -nv --no-iri -N -O $(PACKAGE) 'https://....' since PACKAGE = win32/en-US/<pkgname> #2 is essentially saving win32/en-US/pkgname in the <dist>/win32/en-US path but when you're in <dist>/win32/en-US, win32/en-US doesn't exist as a subdirectory of that. So it fails. Which is why if we don't touch

I forgot to delete the below ;/ Do ignore. > > 1) cd $(ABS_DIST)/$(PKG_PATH) [ <dist>/win32/en-US ] > 2) wget --no-cache -nv --no-iri -N -O $(PACKAGE) 'https://....' > > since PACKAGE = win32/en-US/<pkgname> > > #2 is essentially saving win32/en-US/pkgname in the <dist>/win32/en-US > path but when you're in <dist>/win32/en-US, win32/en-US doesn't exist > as a subdirectory of that. > > So it fails. > > Which is why if we don't touch

It's my understanding that specifying -O $(PACKAGE) is superfluous if EN_US_PACKAGE_NAME is PACKAGE. The problem is, $(PACKAGE) has paths in it which will break the repackage part if where we do the 'wget' to has *nothing* in it. So removing the -O is still ok as it still downloads the EN_US_PACKAGE_NAME to the (basename $(EN_US_PACKAGE_NAME)) to $(ABS_DIST)/$(PKG_PATH). (if given a r+, this needs to be pushed to m-b's RELBRANCH *and* m-i, *and* uplifted to m-a and m-r and m-b proper); but I suppose I'm jumping the gun here.

to note: this is the most simplest patch without getting so convoluted and complicated. Also, this was pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f760bc60c6be0274e3f0b7ce4b0c4794d4cd5e0&selectedJob=83570457

Comment on attachment 8846942 [details] [diff] [review] proposed patch Review of attachment 8846942 [details] [diff] [review]: ----------------------------------------------------------------- I understand the issue now. For us, on the relbranch, I think is probably the shortest and least confusing way to do it. That said, I'm not sure if the original intent of the patch was that the file would have a different name and it would need to be "corrected" to the expected name by specifying -O here. In that case, your patch with basename is probably the right one.

Comment on attachment 8846942 [details] [diff] [review] proposed patch I don't think removing the -O $(PACKAGE) will work since I'm pretty sure it is needed from bug 1301509 in order to grab Taskcluster-named packages (eg: target.bz2 or whatever) and still use the regular packaging machinery with the firefox-foo names. Callek could confirm this if you think this is the only route. Additionally, there are no more uses of PKG_PATH in mozilla-central, so we are planning to remove it in bug 1341291. I think any fix that relies on PKG_PATH still working is likely going to have to be fixed again in the future. What is the value of EN_US_BINARY_URL for you? Does it work if you add win32/en-US/ to EN_US_BINARY_URL and remove it from PACKAGE and then delete PKG_PATH?

(In reply to Michael Shal [:mshal] from comment #24) > Comment on attachment 8846942 [details] [diff] [review] > proposed patch > > I don't think removing the -O $(PACKAGE) will work since I'm pretty sure it > is needed from bug 1301509 in order to grab Taskcluster-named packages (eg: > target.bz2 or whatever) and still use the regular packaging machinery with > the firefox-foo names. Callek could confirm this if you think this is the > only route. Aren't Taskcluster named packages grabbed from the url and -O is where it saves the package? so if you're in foo/bar, and you "wget -O foo/bar/baz.tgz <url>", would you not get an error if foo/bar doesn't exist already? Since you're already in $(ABS_DIST)/$(PKG_PATH) (in m-b, it's still valid), wget -O <pkgname only> <url> would suffice to get the package, no? > > Additionally, there are no more uses of PKG_PATH in mozilla-central, so we > are planning to remove it in bug 1341291. I think any fix that relies on > PKG_PATH still working is likely going to have to be fixed again in the > future. > True. > What is the value of EN_US_BINARY_URL for you? Does it work if you add > win32/en-US/ to EN_US_BINARY_URL and remove it from PACKAGE and then delete > PKG_PATH? Unfortunately, that won't work because in: https://hg.mozilla.org/releases/mozilla-beta/file/tip/toolkit/mozapps/installer/upload-files.mk#l132 it depends on saving to win32/<locale>/packagename. Without the PKG_PATH in PACKAGE, it'll only save the package to the <dist> path and that throws a wrench in the repackage-zip portion since it expects win32/<locale>/packagename to exist.

(In reply to Edmund Wong (:ewong) from comment #25) > (In reply to Michael Shal [:mshal] from comment #24) > > I don't think removing the -O $(PACKAGE) will work since I'm pretty sure it > > is needed from bug 1301509 in order to grab Taskcluster-named packages (eg: > > target.bz2 or whatever) and still use the regular packaging machinery with > > the firefox-foo names. Callek could confirm this if you think this is the > > only route. > > Aren't Taskcluster named packages grabbed from the url and -O is where > it saves the package? so if you're in foo/bar, and you > "wget -O foo/bar/baz.tgz <url>", would you not get an error if foo/bar > doesn't exist already? The Taskcluster packages are named like "target.tar.bz2", but we can't use "target.tar.bz2" as the output for l10n because we build multiple locales at once (chunking), otherwise each locale that we build would just override the previous. Callek's solution to this in bug 1301509 was to not use the Taskcluster simple naming, but then he needed to fix the l10n repackaging code so that it could find the en-US build by essentially renaming it during the wget from "target.tar.bz2" to something like "firefox-53.0.en-US.linux-i686.tar.bz2". It does look like the patch is technically incorrect since as you say we cd into PKG_PATH first, and then wget -O $(PACKAGE), which is defined as $(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX). However since we never set PKG_PATH anymore in firefox, we never hit this error. > Since you're already in $(ABS_DIST)/$(PKG_PATH) (in m-b, it's still valid), > wget -O <pkgname only> <url> would suffice to get the package, no? Yeah, we should probably use '-O $(notdir $(PACKAGE))' instead of '-O $(PACKAGE)' if we need to support this in beta. However, even in beta, firefox still does not set PKG_PATH. There is still some code that appears to set it in package-name.mk, but that is only reached if MOZ_PKG_PRETTYNAMES is set, and we no longer use pretty names anywhere. We ended up removing that from the tree in bug 1329355, but it was already unused before that because of release promotion. Essentially, that logic exists in mozilla-beta but it is dead code (for firefox). I would support replacing -O $(PACKAGE) with -O $(notdir $(PACKAGE)) in mozilla-beta+ if that helps unblock you, but for mozilla-central we're killing PKG_PATH anyway, so there's no reason to use $(notdir) there. > > What is the value of EN_US_BINARY_URL for you? Does it work if you add > > win32/en-US/ to EN_US_BINARY_URL and remove it from PACKAGE and then delete > > PKG_PATH? > > Unfortunately, that won't work because in: > > https://hg.mozilla.org/releases/mozilla-beta/file/tip/toolkit/mozapps/ > installer/upload-files.mk#l132 > > it depends on saving to win32/<locale>/packagename. Without the PKG_PATH in > PACKAGE, it'll only save the package to the <dist> path and that throws > a wrench in the repackage-zip portion since it expects > win32/<locale>/packagename > to exist. Unfortunately I don't have a good solution here. It sounds like it will be difficult to support both l10n styles in the tree. Callek, do you have any suggestions here?

(In reply to Michael Shal [:mshal] from comment #26) > (In reply to Edmund Wong (:ewong) from comment #25) > > (In reply to Michael Shal [:mshal] from comment #24) > > > I don't think removing the -O $(PACKAGE) will work since I'm pretty sure it > > > is needed from bug 1301509 in order to grab Taskcluster-named packages (eg: > > > target.bz2 or whatever) and still use the regular packaging machinery with > > > the firefox-foo names. Callek could confirm this if you think this is the > > > only route. > > > > Aren't Taskcluster named packages grabbed from the url and -O is where > > it saves the package? so if you're in foo/bar, and you > > "wget -O foo/bar/baz.tgz <url>", would you not get an error if foo/bar > > doesn't exist already? > > The Taskcluster packages are named like "target.tar.bz2", but we can't use > "target.tar.bz2" as the output for l10n because we build multiple locales at > once (chunking), otherwise each locale that we build would just override the > previous. Callek's solution to this in bug 1301509 was to not use the > Taskcluster simple naming, but then he needed to fix the l10n repackaging > code so that it could find the en-US build by essentially renaming it during > the wget from "target.tar.bz2" to something like > "firefox-53.0.en-US.linux-i686.tar.bz2". > > It does look like the patch is technically incorrect since as you say we cd > into PKG_PATH first, and then wget -O $(PACKAGE), which is defined as > $(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX). However since we never set PKG_PATH > anymore in firefox, we never hit this error. Fwiw, this is for Gecko 51 beta and not the current Gecko beta (which is 53, right?). > > > Since you're already in $(ABS_DIST)/$(PKG_PATH) (in m-b, it's still valid), > > wget -O <pkgname only> <url> would suffice to get the package, no? > > Yeah, we should probably use '-O $(notdir $(PACKAGE))' instead of '-O > $(PACKAGE)' if we need to support this in beta. However, even in beta, > firefox still does not set PKG_PATH. There is still some code that appears > to set it in package-name.mk, but that is only reached if > MOZ_PKG_PRETTYNAMES is set, and we no longer use pretty names anywhere. We > ended up removing that from the tree in bug 1329355, but it was already > unused before that because of release promotion. Essentially, that logic > exists in mozilla-beta but it is dead code (for firefox). Ah ok. I'll keep that in mind. > > I would support replacing -O $(PACKAGE) with -O $(notdir $(PACKAGE)) in > mozilla-beta+ if that helps unblock you, but for mozilla-central we're > killing PKG_PATH anyway, so there's no reason to use $(notdir) there. I know about the m-c removal and I think at that point, we'll need to get rid of the MOZ_PKG_PRETTYNAMES as well. Thanks!

(In reply to Edmund Wong (:ewong) from comment #27) > (In reply to Michael Shal [:mshal] from comment #26) > > It does look like the patch is technically incorrect since as you say we cd > > into PKG_PATH first, and then wget -O $(PACKAGE), which is defined as > > $(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX). However since we never set PKG_PATH > > anymore in firefox, we never hit this error. > > Fwiw, this is for Gecko 51 beta and not the current Gecko beta (which is > 53, right?). Ahh, I don't remember the state of things in 51. Does the $(notdir) solution work for you there?

(In reply to Michael Shal [:mshal] from comment #28) > (In reply to Edmund Wong (:ewong) from comment #27) > > (In reply to Michael Shal [:mshal] from comment #26) > > > It does look like the patch is technically incorrect since as you say we cd > > > into PKG_PATH first, and then wget -O $(PACKAGE), which is defined as > > > $(PKG_PATH)$(PKG_BASENAME)$(PKG_SUFFIX). However since we never set PKG_PATH > > > anymore in firefox, we never hit this error. > > > > Fwiw, this is for Gecko 51 beta and not the current Gecko beta (which is > > 53, right?). > > Ahh, I don't remember the state of things in 51. Does the $(notdir) solution > work for you there? Yes, the $(notdir) solution works perfectly!

ensure paths are not used in the PACKAGE variable.

Comment on attachment 8848389 [details] [diff] [review] proposed patch (v2) LGTM! Where all does this need to land?

(In reply to Michael Shal [:mshal] from comment #31) > Comment on attachment 8848389 [details] [diff] [review] > proposed patch (v2) > > LGTM! Where all does this need to land? This needs to land first in m-b Relbranch(gecko 51) and Gecko 51's m-r. Then the following: 1) Gecko 52's m-b, and m-esr52 (optional Gecko 52 m-r for good measure) 2) Gecko 53's m-b, and Gecko 54 m-a. :mshal, is this even do-able? Rationale (if someone can clarify if I have this right): This bug is due to a change in Gecko 51. Which means it's in 52, 53, 54, and 55. And I would need to have this patch pushed to all those trees listed above to fix the repacks issue. The reason why I'll request all those branches is because SeaMonkey (currently spinning 2.48b1) will do the following releases (after 2.48b1): 1) 2.48 release [corresponds to Gecko 51 m-r] 2) 2.49b1 [ corresponds to Gecko 52 m-b] 3) 2.49esr release [corresponds to Gecko 52 m-esr] 4) 2.50b1 [ corresponds to Gecko 53 m-b] Since we're that behind, we need some 'slack time' to catch up and asking it to be pushed to Gecko 54 m-a will at least give us some breathing space such that we won't need to worry about 2.51b1 busting us because of this. As noted, I will push this patch to the SeaMonkey relbranch (which doesn't require M-b approval as it's a relbranch). Then I'll attempt to request the other branches (I'll even create relbranches on those old 52 m-b branch if it's ok). [tbh, not even sure how I'll even accomplish the requests.] Can someone loop in a Rel manager here for some insight?

> Unfortunately I don't have a good solution here. It sounds like it will be > difficult to support both l10n styles in the tree. Callek, do you have any > suggestions here? Looks like you guys found a solution (In reply to Edmund Wong (:ewong) from comment #32) > (In reply to Michael Shal [:mshal] from comment #31) > > Comment on attachment 8848389 [details] [diff] [review] > > proposed patch (v2) > > > > LGTM! Where all does this need to land? > ... > Can someone loop in a Rel manager here for some insight? I'm not a manager, but you shouldn't need any explicit signoff for creating and landing on relbranches, please just (if possible) push them all at once, and let releng know in #releaseduty (just incase it trips some weird state in releasy stuff, but it shouldn't aiui)

I don't know about landing things on previously released versions (eg: beta 51). Looping in Sylvestre for guidance on #c32.

You should use the relbranch for that

(In reply to Sylvestre Ledru [:sylvestre] from comment #35) > You should use the relbranch for that Thanks :sylvestre.. I'll push this to all the relbranches for those old trees.

(In reply to Michael Shal [:mshal] from comment #34) > I don't know about landing things on previously released versions (eg: beta > 51). Looping in Sylvestre for guidance on #c32. :mshal, I didn't realize that the mac package had space in it and so this patch doesn't work for the mac platform. I need to add quotes to the $(notdir $(PACKAGE)).

Comment on attachment 8852285 [details] [diff] [review] additional patch to fix mac package names Review of attachment 8852285 [details] [diff] [review]: ----------------------------------------------------------------- I see no reason this would break (so to speak)...

Fixed