Closed Bug 451461 Opened 13 years ago Closed 13 years ago

make target that prepares l10n repackages to upload to ftp

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: armenzg)

References

Details

(Keywords: fixed1.9.0.4)

Attachments

(2 files, 5 obsolete files)

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)
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
Status: NEW → ASSIGNED
(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.


It generates the folders that I need, dated and latest
Attachment #334791 - Attachment is obsolete: true
Attachment #334974 - Flags: review?(ted.mielczarek)
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
Attachment #334974 - Attachment is obsolete: true
Attachment #334977 - Flags: review?(ted.mielczarek)
Attachment #334974 - Flags: review?(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.
Attachment #334977 - Flags: review?(ted.mielczarek) → review-
(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

I have fixed the chmod and changed the "ln -s" for "mv" instead
Attachment #334977 - Attachment is obsolete: true
Attachment #335130 - Flags: review?(ted.mielczarek)
Attachment #335130 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
Whiteboard: [needs checkin on 1.9.0]
is the whiteboard note correct?
[needs checkin on 1.9.1 and 1.9.0]??
Whiteboard: [needs checkin on 1.9.0] → [needs checkin on 1.9.1 and 1.9.0]
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
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: [needs checkin on 1.9.1 and 1.9.0] → [needs checkin on 1.9.1]
Attachment #335130 - Flags: approval1.9.0.2?
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
Attachment #335130 - Flags: approval1.9.0.2? → review-
Keywords: checkin-needed
Whiteboard: [needs checkin on 1.9.1]
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
Attachment #335130 - Attachment is obsolete: true
Attachment #336402 - Flags: review?(reed)
Blocks: 451088
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?
Attachment #336402 - Flags: review?(reed)
Assignee: armenzg → ccooper
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 ?
(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
fixed

replace
(else if condition)
for
(else
if condition)
Attachment #336402 - Attachment is obsolete: true
Attachment #337773 - Flags: review?(nthomas)
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).
Attachment #337773 - Flags: review?(nthomas) → review?(ted.mielczarek)
(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.
(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
Attachment #337773 - Flags: review?(ted.mielczarek) → review+
please checkin-needed for cvs head and hg
Keywords: checkin-needed
There has been a commit in hg that has not happened in cvs since attachment 33773 [details] [diff] [review]  was created.
Attachment #338871 - Flags: review+
Attachment #338871 - Flags: approval1.9.0.3?
(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.
(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.
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.
(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...
Assignee: ccooper → armenzg
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
Attachment #338871 - Flags: approval1.9.0.3? → approval1.9.0.3+
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
Attachment #338871 - Attachment description: the same patch as the previous one but for CVS → [checked in]the same patch as the previous one but for CVS
Still need to land in hg, but the change has been pushed to CVS HEAD now.
Keywords: fixed1.9.0.3
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.
Attachment #337773 - Attachment description: prepare repackages in correct structure to upload to ftp server → prepare repackages in correct structure to upload to ftp server [Checkin: Comment 28]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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?
(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
(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.
Blocks: 456351
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.