351476 - Remove post-mozilla-rel.pl conversion of line endings in browser/EULA and other files

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect, P3)

Description

[Robert Strong (they/them - no direct email)]

We aren't using this file atm and will probably be removing it in the future due to:
Win32 / NSIS uses EULA.rtf
Mac OS X uses license.r
Linux doesn't display a EULA due to being a tar

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 236872 [details] [diff] [review]
patch

bah... as is this could break 1.5.0.x due to it using the same script

Comment 3

[Robert Strong (they/them - no direct email)]

morphing this bug to cover all of the following... we should just do this during the build on win32 instead of modifying the source with post-mozilla-rel.pl.

if (TinderUtils::is_windows()) {
  # hack for cygwin installs with "unix" filetypes
  TinderUtils::run_shell_command("unix2dos $mozilla_build_dir/mozilla/LICENSE");
  TinderUtils::run_shell_command("unix2dos $mozilla_build_dir/mozilla/mail/LICENSE.txt");
  TinderUtils::run_shell_command("unix2dos $mozilla_build_dir/mozilla/README.txt");
  TinderUtils::run_shell_command("unix2dos $mozilla_build_dir/mozilla/browser/EULA");
}

Comment 5

[Robert Strong (they/them - no direct email)]

over to default owner in case someone has time to fix this

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: rough first shot

still testing this so not asking for review; if we get this working on trunk and branch we can remove the unix2dos crud in tinderbox.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to comment #0)
> We aren't using this file atm and will probably be removing it in the future
> due to:
> Win32 / NSIS uses EULA.rtf
> Mac OS X uses license.r
> Linux doesn't display a EULA due to being a tar


Hmm.. so no point in converting EULA or LICENSE.txt if it's not used then right? Is README.txt, on win32, the only file that needs EOL conversion then?

Comment 8

[Robert Strong (they/them - no direct email)]

EULA isn't distributed but LICENSE.txt is as LICENSE in the installation directory. So I'd say LICENSE.txt and README.txt both need to be converted.

Comment 9

[Robert Helmer [:rhelmer]]

Attachment: convert EOL on README/LICENSE after copying from srcdir for Firefox

Note that creating a backup file ("-ibak") prevents this on the tryserver, which is running the latest win32 ref vm (msys):

Can't do inplace edit on ../../dist/bin/LICENSE: Permission denied.

As far as I can tell, inplace editing just doesn't work on Windows from Perl without making a backup file on the ref platform (perl 5.6.1); in fact it ends up just deleting the file (!).

Comment 10

[Benjamin Smedberg]

Comment on attachment 282842 [details] [diff] [review]
convert EOL on README/LICENSE after copying from srcdir for Firefox

>Index: browser/locales/Makefile.in

> libs:: $(addprefix $(LOCALE_SRCDIR)/,$(README_FILES))
> 	$(SYSINSTALL) $(IFLAGS1) $^ $(FINAL_TARGET)
>+ifeq ($(OS_ARCH), WINNT)
>+	perl -ibak -pe 's/(?<!\r)\n/\r\n/g;' \
>+	$(addprefix $(FINAL_TARGET),/$(README_FILES))
>+endif

Note: the space before WINNT will probably mess things up...

Rather than installing and then in-place editing, the easier/better way to do this is:

ifeq ($(OS_ARCH),WINNT)
	for file in $^; do
	  $(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $file > $(FINAL_TARGET)/`basename $file`;
	done
else
	$(SYSINSTALL) $(IFLAGS1) $^ $(FINAL_TARGET)
endif

That also won't clutter dist/bin with .bak files.

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: convert EOL on README/LICENSE, do not edit inplace

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: convert EOL on README/LICENSE, do not edit inplace

might be nice if I specified the correct srcdir for LICENSE :)

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: [checked in]convert EOL on README/LICENSE, do not edit inplace

Comment 14

[Benjamin Smedberg]

Comment on attachment 283208 [details] [diff] [review]
[checked in]convert EOL on README/LICENSE, do not edit inplace

>Index: browser/locales/Makefile.in

>+ifeq ($(OS_ARCH),WINNT)
>+	for file in $^; do \
>+	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $$file > $(FINAL_TARGET)/`basename $$file`; \
>+	done

Please add two spaces to the middle line so that it's <tab><space><space> indented relative to the "for" statement.

r=me

Comment 15

[Benjamin Smedberg]

Oh also, please add an EXIT_ON_ERROR bit to this; for statements hide failure codes by default.

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to comment #14)
> (From update of attachment 283208 [details] [diff] [review])
> >Index: browser/locales/Makefile.in
> 
> >+ifeq ($(OS_ARCH),WINNT)
> >+	for file in $^; do \
> >+	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $$file > $(FINAL_TARGET)/`basename $$file`; \
> >+	done
> 
> Please add two spaces to the middle line so that it's <tab><space><space>
> indented relative to the "for" statement.
> 
> r=me
> 

Done and landed (before I got comment #15):
Checking in browser/locales/Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.54; previous revision: 1.53
done
Checking in browser/app/Makefile.in;
/cvsroot/mozilla/browser/app/Makefile.in,v  <--  Makefile.in
new revision: 1.138; previous revision: 1.137
done

Comment 17

[Robert Helmer [:rhelmer]]

Attachment: [checked in]add EXIT_ON_ERROR for EOL conversion

Comment 18

[Benjamin Smedberg]

Comment on attachment 283235 [details] [diff] [review]
[checked in]add EXIT_ON_ERROR for EOL conversion

>Index: browser/app/Makefile.in

> libs::
> ifeq ($(OS_ARCH),WINNT)
>+	$(EXIT_ON_ERROR) \
> 	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $(topsrcdir)/LICENSE > $(DIST)/LICENSE

No need for EXIT_ON_ERROR here since you've only got a single command running

Comment 19

[Robert Helmer [:rhelmer]]

(In reply to comment #18)
> (From update of attachment 283235 [details] [diff] [review])
> >Index: browser/app/Makefile.in
> 
> > libs::
> > ifeq ($(OS_ARCH),WINNT)
> >+	$(EXIT_ON_ERROR) \
> > 	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $(topsrcdir)/LICENSE > $(DIST)/LICENSE
> 
> No need for EXIT_ON_ERROR here since you've only got a single command running
> 

Thanks, just landed the change to browser/locales/Makefile.in:

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

Comment 20

[Robert Helmer [:rhelmer]]

Actually, we didn't remove anything from post-mozilla-rel.pl yet so leaving this open.

In order to do that I think we'll need to get this patch on the branch, and also the other projects that use Tinderbox for win32 builds (Thunderbird, Seamonkey, Calendar at least).

Comment 21

[Phil Ringnalda (:philor)]

Comment on attachment 283208 [details] [diff] [review]
[checked in]convert EOL on README/LICENSE, do not edit inplace

>Index: browser/app/Makefile.in
>+ifeq ($(OS_ARCH),WINNT)
>+	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $(topsrcdir)/LICENSE > $(DIST)/LICENSE
>+else
> 	$(INSTALL) $(topsrcdir)/LICENSE	$(DIST)/bin

$(DIST)/LICENSE for Windows, $(DIST)/bin/LICENSE otherwise? packages-static is still looking (in vain) for it in bin/LICENSE, according to the top of the tinderbox brief log.

Comment 22

[Robert Helmer [:rhelmer]]

Attachment: [checked in] copy to dist/bin/LICENSE not dist/LICENSE

Ouch! good catch, thanks philor:

Checking in browser/app/Makefile.in;
/cvsroot/mozilla/browser/app/Makefile.in,v  <--  Makefile.in
new revision: 1.139; previous revision: 1.138
done

Comment 23

[Robert Helmer [:rhelmer]]

Returning to the default assignee for now. 
Still needed if we want to remove this from Tinderbox:

* port the build system change here to other products
* merge the change to Firefox branches
* remove conversion from post-mozilla-rel.pl in Tinderbox

Alternatively, could special-case Firefox in post-mozilla-rel.pl, but that's messy and I don't think there's any reason not to do it right (the above 3 bullet points).

Comment 24

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

Attachment: [checked in] Convert EOL on README/LICENSE, do not edit inplace (Mail)

Comment 25

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

Attachment: [checked in] Convert EOL on README/LICENSE, do not edit inplace (SeaMonkey)

Comment 26

[Scott MacGregor]

Comment on attachment 300409 [details] [diff] [review]
[checked in] Convert EOL on README/LICENSE, do not edit inplace (Mail)

thanks coop.

Comment 27

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

Attachment: Convert EOL on README/LICENSE, do not edit inplace (Calendar)

Comment 28

[ause]

(In reply to comment #27)
> Created an attachment (id=300412) [details]
> Convert EOL on README/LICENSE, do not edit inplace (Calendar)
> 
the patch is fine but the changed part in calendar/locales/Makefile.in is just on the move to calendar/sunbird/locales/Makefile.in (Bug 413868). i'll to get that coordinated.

Comment 29

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

Attachment: [checked in] Remove unix2dos calls in post-mozilla-rel.pl

Comment 30

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

Any of the affected projects (Thunderbird, SeaMonkey, Calendar) object to my landing the Makefile.in changes this week?

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Convert EOL on README/LICENSE, do not edit inplace (Calendar)

Calendar changed the location where the README file is processed, this patch takes care.

Carrying forth r+ since only file location changed. Also checking in on HEAD, MOZILLA_1_8_BRANCH, and SUNBIRD_0_8_BRANCH (we are shortly before release phase).

Comment 32

[Robert Helmer [:rhelmer]]

Comment on attachment 307227 [details] [diff] [review]
[checked in] Remove unix2dos calls in post-mozilla-rel.pl

yay

Comment 33

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

Comment on attachment 307227 [details] [diff] [review]
[checked in] Remove unix2dos calls in post-mozilla-rel.pl

Checking in post-mozilla-rel.pl;
/cvsroot/mozilla/tools/tinderbox/post-mozilla-rel.pl,v  <--  post-mozilla-rel.pl
new revision: 1.142; previous revision: 1.141
done

Comment 34

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

All the trunk patches have landed. I'll work up branch patches for Firefox and Thunderbird tonight/tomorrow.

Comment 35

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

Attachment: Convert EOL on README/LICENSE, do not edit inplace (Firefox 1.8 branch)

Comment 36

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

Attachment: Convert EOL on README/LICENSE, do not edit inplace (Thunderbird 1.8 branch)

Comment 37

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

benjamin: need sr? and a? to land these 1.8 patches?

Comment 38

[Benjamin Smedberg]

need a

Comment 39

[Daniel Veditz [:dveditz]]

Why do we need this on the 1.8 branch? Is it really a blocker, or a "nice to have"?

To check in you need "approval1.8.1.15+" on a patch, regardless of whether the bug is a blocker or not. We do approve non-blockers, at least early in the release.(see rules on the tinderbox page)

Comment 40

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

(In reply to comment #39)
> Why do we need this on the 1.8 branch? Is it really a blocker, or a "nice to
> have"?

It's not technically a blocker, it just allows us to clean up our build scripts a bit.
 
> To check in you need "approval1.8.1.15+" on a patch, regardless of whether the
> bug is a blocker or not. We do approve non-blockers, at least early in the
> release.(see rules on the tinderbox page)

If someone want to offer up an approval, that's great. I can't even set the flag to ask for one it seems.

Comment 41

[Daniel Veditz [:dveditz]]

(In reply to comment #40)
> > To check in you need "approval1.8.1.15+" on a patch
> 
> If someone want to offer up an approval, that's great. I can't even set the
> flag to ask for one it seems.

Not in the "mozilla.org" Product, no. Those flags only exist in the Client-oriented products where they make sense. If this bug is changing code in the mozilla client source tree maybe it should be moved to the "Core:Build config" product:component.

Comment 42

[Daniel Veditz [:dveditz]]

Not blocking but will consider approving patches.

Comment 43

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

(In reply to comment #42)
> Not blocking but will consider approving patches.

Can you approve the last two patches above? They are already r+'d by bsmedberg, and are all we need to land on the 1.8 branch.

Comment 44

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

Worked with dveditz to spin off bug#432311 for the 1.8 branch landings.



Is there anything left to do in this bug?

Comment 45

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

Dependent bugs for branch landings now closed, so marking as FIXED.