451461 - make target that prepares l10n repackages to upload to ftp

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server

This has been separated from the wget patch from Bug 445254

The make target sets a variable UPLOAD_DIR which is a folder where we copy the recently generated repackages, xpi (langpacks) and/or windows installers to the specified folder to uploaded from there to the ftp server in the nightly section

The structure followed is (in this case windows):
- dist/upload/latest/firefox-3.0.2pre.af.win32.zip
- dist/upload/latest/firefox-3.0.2pre.af.win32.installer.exe
- dist/upload/latest/windows-xpi/af.xpi

In the prepare-upload-dated, we specify the dated folder we want:
UPLOAD_DIR = $(UPLOAD_DIR)/upload/.`date "+%y-%m-%d-%H"`.-$(MOZ_APP_VERSION)-l10n

It seems that the dated folder is where we pull from in releases(check the mentioned Bug 445254)

Comment 1

[Armen [:armenzg]]

BTW, I have separated this patch from the(In reply to comment #0)
> Created an attachment (id=334791) [details]
> prepare repackages in correct structure to upload to ftp server
> 
> This has been separated from the wget patch from Bug 445254
>
BTW, I have separated this patch from the wget because I had to do more changes in this one and the wget one was ready to be landed

Comment 2

[Armen [:armenzg]]

(In reply to comment #0)
> It seems that the dated folder is where we pull from in releases(check the
> mentioned Bug 445254)
> 
As mentioned in bug 451225#c3
>If your code pushes to the right place on stage to begin with, eg,
>/home/ftp/pub/firefox/nightly/2008-07-24-20-firefox3.0.2-l10n/, these steps
>will work just fine.

Comment 3

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server

It generates the folders that I need, dated and latest

Comment 4

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server

I forgot to add the chmod line which is in use in staging-1.9 and I have added comments and the order of the make targets

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 334977 [details] [diff] [review]
prepare repackages in correct structure to upload to ftp server

+prepare-upload-dated-%:
+	@$(MAKE) prepare-repackages-$* UPLOAD_DIR=$(DIST)/upload/`date "+%Y-%m-%d-%H"`-$(MOZ_PKG_APPNAME)$(MOZ_APP_VERSION)-l10n

Is using 'date' really the right thing to do here? Are the dated dirs just supposed to be "whatever the current date is", or should they be based on the BuildID or something?

+# Link to the langpack
+	ln -s $(DIST)/install/firefox-$(MOZ_APP_VERSION).$*.langpack.xpi \
+	   $(UPLOAD_DIR)/$(XPI_DESTINATION)/$*.xpi

I don't think you should use 'ln -s' explicitly here. Why not use $(INSTALL)? That should symlink on platforms that support it.

+# Set the permissions that the folders will have in ftp once uploaded
+	chmod -vR 755 $(UPLOAD_DIR)

I really doubt that this will work on Windows. Have you tried it there? MSYS doesn't really have a good concept of unix permissions.

Comment 6

[Armen [:armenzg]]

(In reply to comment #5)
> (From update of attachment 334977 [details] [diff] [review])
> +prepare-upload-dated-%:
> +       @$(MAKE) prepare-repackages-$* UPLOAD_DIR=$(DIST)/upload/`date
> "+%Y-%m-%d-%H"`-$(MOZ_PKG_APPNAME)$(MOZ_APP_VERSION)-l10n
> 
> Is using 'date' really the right thing to do here? Are the dated dirs just
> supposed to be "whatever the current date is", or should they be based on the
> BuildID or something?
> 
This is where is gets generated the folder name
http://mxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#1397
1390     if ($Settings::BuildLocales) {
1391       my ($c_hour,$c_day,$c_month,$c_year,$c_yday) = (localtime(time))[2,3,4,5,7];
1392       $c_year       = $c_year + 1900; # ftso perl
1393       $c_month      = $c_month + 1; # ftso perl
1394       $c_hour       = pad_digit($c_hour);
1395       $c_day        = pad_digit($c_day);
1396       $c_month      = pad_digit($c_month);
1397       $datestamp = "$c_year-$c_month-$c_day-$c_hour-$Settings::milestone";
and
1411     $package_dir = "$datestamp";
which is passed to:
1496   unless (pushit(server => $Settings::ssh_server, remote_path => $ftp_path, package_name => $package_dir,


> +# Link to the langpack
> +       ln -s $(DIST)/install/firefox-$(MOZ_APP_VERSION).$*.langpack.xpi \
> +          $(UPLOAD_DIR)/$(XPI_DESTINATION)/$*.xpi
> 
> I don't think you should use 'ln -s' explicitly here. Why not use $(INSTALL)?
> That should symlink on platforms that support it.
> 
I think I could keep it as "move" as I had it before since it makes no sense to want to prepare the folder structure for nightly when we are doing a release. Like calling "prepare-upload-latest" and just after that "prepare-upload-dated"

> +# Set the permissions that the folders will have in ftp once uploaded
> +       chmod -vR 755 $(UPLOAD_DIR)
> 
> I really doubt that this will work on Windows. Have you tried it there? MSYS
> doesn't really have a good concept of unix permissions.
> 
This has been working in staging 1.9 for all 3 platforms
We have as well using chmod in the tinderbox code:
http://mxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#1115
I have realized that is 775 instead of 755

Comment 7

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server

I have fixed the chmod and changed the "ln -s" for "mv" instead

Comment 8

[Armen [:armenzg]]

is the whiteboard note correct?
[needs checkin on 1.9.1 and 1.9.0]??

Comment 9

[Reed Loden [:reed]]

CVS HEAD:

Checking in browser/locales/Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.62; previous revision: 1.61
done

Comment 10

[Reed Loden [:reed]]

Comment on attachment 335130 [details] [diff] [review]
prepare repackages in correct structure to upload to ftp server

This doesn't work.

/builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/config/nsinstall -L /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/browser/components/build -m 644 /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/mozilla/browser/components/build/nsBrowserCompsCID.h ../../../dist/include/browsercomps
/builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/config/nsinstall -L /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/browser/components -m 644 /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/mozilla/browser/components/nsIBrowserHandler.idl /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/mozilla/browser/components/nsIBrowserGlue.idl ../../dist/idl
/builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/config/nsinstall -L /builds/tinderbox/Fx-Trunk/Darwin_8.8.4_Depend/build/universal/ppc/browser/components -m 644 _xpidlgen/nsIBrowserHandler.h _xpidlgen/nsIBrowserGlue.h ../../dist/include/browsercomps 
Makefile:340: Extraneous text after `else' directive
Makefile:342: Extraneous text after `else' directive
Makefile:342: *** only one `else' per conditional.  Stop.
make[5]: *** [export] Error 2
make[4]: *** [export_tier_app] Error 2
make[3]: *** [tier_app] Error 2
make[2]: *** [alldep] Error 2
make[1]: *** [alldep] Error 2
make: *** [alldep] Error 2

Comment 11

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server

I have made no changes to the patch, the only things is that appears at a higher line number since there has been a commit since last time

Is the log that you show from a FF3.0 build?

The only problem that I can see from the previous patch was that it would have not applied correctly. It doesn't make sense the bustage
Let me use the try server and see if I can get the same log you got.

could this please be checked in to hg?
I will ask to be checked in to CVS HEAD once there is approval1.9.0.x rather than before

Comment 12

[Armen [:armenzg]]

Comment on attachment 336402 [details] [diff] [review]
 prepare repackages in correct structure to upload to ftp server

could someone please check this in to hg?

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

I think the error in comment #10 (which is from a 3.0 build) is not a patch issue, it's make 3.80 objecting to anything following |else|. Or to put it another way, 3.80 only supports
     conditional-directive
     text-if-one-is-true
     else
     text-if-false
     endif
and not 
     conditional-directive
     text-if-one-is-true
     else conditional-directive
     text-if-true
     else
     text-if-false
     endif

We have make 3.80 on Fx3.0 and older macs; 3.81 on the pool for mozilla-central et al. We could land different patches in the two places, but I think it's better to land the same patch in both. Can you rewrite this to achieve the same effect ?

Comment 14

[Armen [:armenzg]]

(In reply to comment #13)
> We could land different patches in the two places, but I think it's
> better to land the same patch in both. Can you rewrite this to achieve the same
> effect ?
I will rewrite it

Comment 15

[Armen [:armenzg]]

Attachment: prepare repackages in correct structure to upload to ftp server [Checkin: Comment 28]

fixed

replace
(else if condition)
for
(else
if condition)

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 337773 [details] [diff] [review]
prepare repackages in correct structure to upload to ftp server
[Checkin: Comment 28]

Changing review to owner of this code (the build system).

Comment 17

[Chris Cooper [:coop] (he/him)]

(In reply to comment #16)
> (From update of attachment 337773 [details] [diff] [review])
> Changing review to owner of this code (the build system).

Armen: can you give Ted a quick rundown of the testing you've done and the results you've seen trying this patch locally since reed's comment 10? It might expedite a review. Thanks.

Comment 18

[Armen [:armenzg]]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 337773 [details] [diff] [review] [details])
> > Changing review to owner of this code (the build system).
> 
> Armen: can you give Ted a quick rundown of the testing you've done and the
> results you've seen trying this patch locally since reed's comment 10? It might
> expedite a review. Thanks.

They are all green.

These are the logs of the patch for cvs:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221020658.1221022703.16694.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221024251.1221026606.26881.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry

These are the logs for hg:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221019107.1221023674.18970.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221019214.1221021752.14613.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1221019107.1221022939.17263.gz&fulltext=1

Comment 19

[Armen [:armenzg]]

please checkin-needed for cvs head and hg

Comment 20

[Armen [:armenzg]]

Attachment: [checked in]the same patch as the previous one but for CVS

There has been a commit in hg that has not happened in cvs since attachment 33773 [details] [diff] [review]  was created.

Comment 21

[Chris Cooper [:coop] (he/him)]

(In reply to comment #20)
> Created an attachment (id=338871) [details]
> the same patch as the previous one but for CVS
> 
> There has been a commit in hg that has not happened in cvs since attachment
> 33773 [details]  was created.

We can carry forward the review from Ted, but we still need to get approval to land this on CVS trunk.

Comment 22

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #19)
> please checkin-needed for cvs head and hg

Can we land attachment#337773 [details] [diff] [review] in hg? Its been r+'d, and having this landed in hg would help with the approval1.9.0.3? request.

Comment 23

[Chris Cooper [:coop] (he/him)]

I've been trying, but the tree has been red and/or closed since Tuesday. I'm monitoring tinderbox and will check it in at the first available opportunity.

Comment 24

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #23)
> I've been trying, but the tree has been red and/or closed since Tuesday. I'm
> monitoring tinderbox and will check it in at the first available opportunity.

cool, thanks coop! Armen and I are just sorting through all these l10n bugs, making sure they are uptodate...

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 338871 [details] [diff] [review]
[checked in]the same patch as the previous one but for CVS

Approved for 1.9.0.3, a=dveditz for release-drivers

Comment 26

[Chris Cooper [:coop] (he/him)]

Comment on attachment 338871 [details] [diff] [review]
[checked in]the same patch as the previous one but for CVS

Checking in Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.66; previous revision: 1.65
done

Comment 27

[Chris Cooper [:coop] (he/him)]

Still need to land in hg, but the change has been pushed to CVS HEAD now.

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 337773 [details] [diff] [review]
prepare repackages in correct structure to upload to ftp server
[Checkin: Comment 28]

http://hg.mozilla.org/mozilla-central/rev/ccce859a94aa

I "fixed"
[
Hunk #1 FAILED at 333
]
which was caused by the context only.

I removed some trailing spaces too.

Comment 29

[Robert Kaiser]

Comment on attachment 337773 [details] [diff] [review]
prepare repackages in correct structure to upload to ftp server
[Checkin: Comment 28]

>+# This target will generate a UPLOAD_DIR folder with 
>+# l10n repackages in the way that we offer l10n nightlies 
>+#  1) ./ the binary 
>+#  2) ./{linux,mac,windows}-xpi/locale.xpi 

using {linux,mac,windows}-xpi/ is actually a bug, as langpacks aren't different for the platforms. At least on trunk, we should move away from that.

>+	mv $(DIST)/install/firefox-$(MOZ_APP_VERSION).$*.langpack.xpi \
>+	   $(UPLOAD_DIR)/$(XPI_DESTINATION)/$*.xpi
>+# Move the repackage
>+	mv $(DIST)/firefox-$(MOZ_APP_VERSION).$*.* \
>+	   $(UPLOAD_DIR)/.
>+# Move the windows installer
>+ifeq (WINNT, $(OS_ARCH))
>+	mv $(DIST)/install/sea/firefox-$(MOZ_APP_VERSION).$*.win32.installer.exe \
>+	   $(UPLOAD_DIR)/.

Ugh. Could you please update those to use the new variables for the paths and package names as set by package-name.mk?

Comment 30

[Armen [:armenzg]]

(In reply to comment #29)
> (From update of attachment 337773 [details] [diff] [review])
> >+# This target will generate a UPLOAD_DIR folder with 
> >+# l10n repackages in the way that we offer l10n nightlies 
> >+#  1) ./ the binary 
> >+#  2) ./{linux,mac,windows}-xpi/locale.xpi 
> 
> using {linux,mac,windows}-xpi/ is actually a bug, as langpacks aren't different
> for the platforms. At least on trunk, we should move away from that.
> 
I am so glad I can hear comments like this (revealing more light to my un-knowledge) ;)
Is there a bug filed for this?


> >+	mv $(DIST)/install/firefox-$(MOZ_APP_VERSION).$*.langpack.xpi \
> >+	   $(UPLOAD_DIR)/$(XPI_DESTINATION)/$*.xpi
> >+# Move the repackage
> >+	mv $(DIST)/firefox-$(MOZ_APP_VERSION).$*.* \
> >+	   $(UPLOAD_DIR)/.
> >+# Move the windows installer
> >+ifeq (WINNT, $(OS_ARCH))
> >+	mv $(DIST)/install/sea/firefox-$(MOZ_APP_VERSION).$*.win32.installer.exe \
> >+	   $(UPLOAD_DIR)/.
> 
> Ugh. Could you please update those to use the new variables for the paths and
> package names as set by package-name.mk?
I filed bug 456351 for this

Comment 31

[Robert Kaiser]

(In reply to comment #30)
> > using {linux,mac,windows}-xpi/ is actually a bug, as langpacks aren't different
> > for the platforms. At least on trunk, we should move away from that.
> > 
> I am so glad I can hear comments like this (revealing more light to my
> un-knowledge) ;)
> Is there a bug filed for this?

I don't think so - I fixed it for the case of releases in my "pretty" filename work for the hg-based release, I don't think there's a bug for nightly boxes, but we should go and just only upload them from one platform (e.g. Linux) and into a generic place (just next to the other L10n files?), for hg trunk at least.

> I filed bug 456351 for this

Thanks, more comments there.