Opened 4 months ago

Closed 4 months ago

#26073 closed defect (fixed)

patch tor-browser-build.git for Firefox 60 ESR

Reported by: arthuredelstein Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201805R, ff60-esr
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This ticket is to track progress on getting our builds working based on Firefox 60 ESR.

Child Tickets

TicketStatusOwnerSummaryComponent
#26165closedtbb-teamMake it possible to use gcc:var/setup without hardening wrapperApplications/Tor Browser

Attachments (1)

0001-bug-26073-Migrate-general.useragent.locale-to-intl.l.patch (1.6 KB) - added by igt0 4 months ago.

Download all attachments as: .zip

Change History (48)

comment:1 Changed 4 months ago by arthuredelstein

Here's my current branch for Linux. It fully builds for multiple locales, and runs and connects to https://check.torproject.org.

https://github.com/arthuredelstein/tor-browser-build/commits/26073 (237be3175b514fc6a5df18ecd37816388bfe62f4)

I'm posting it in case anyone wants to work on specific problems in parallel while I continue to refine these patches. The branch uses:

  • the latest torbutton branch #25013
  • the latest tor-launcher branch from #25750
  • the latest tor-browser.git branch from #25543
  • the latest stable noscript
  • the current https-everywhere
  • rust for building Stylo
  • language packs from a Firefox 60 build.

Some obvious things are broken, including:

  • self-rando (I temporarily disabled it.)
  • torbutton (circuit display doesn't work, security slider can't work because noscript has changed)
  • noscript button does not appear on the toolbar

I will shortly post links here to builds for manual testing.

comment:3 in reply to:  1 ; Changed 4 months ago by cypherpunks

Replying to arthuredelstein:

Some obvious things are broken, including:

Torlauncher as well:

https://i.stack.imgur.com/pH5kS.png

Stream isolation broken.

Also nice read :) https://www.morbo.org/2018/05/linux-sandboxing-improvements-in_10.html

Last edited 4 months ago by cypherpunks (previous) (diff)

comment:4 in reply to:  3 Changed 4 months ago by mcs

Replying to cypherpunks:

Replying to arthuredelstein:

Some obvious things are broken, including:

Torlauncher as well:

https://i.stack.imgur.com/pH5kS.png

That Tor Launcher problem is caused by the issue described in #26039.

comment:5 Changed 4 months ago by sukhbir

Other tasks:

  • actually using an esr version of the langpacks
  • figuring out how to get the llvm signature check to work again

comment:6 Changed 4 months ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed

comment:7 Changed 4 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed

comment:8 Changed 4 months ago by gk

Priority: MediumVery High

comment:9 Changed 4 months ago by sukhbir

figuring out how to get the llvm signature check to work again

For review:

https://github.com/arthuredelstein/tor-browser-build/pull/1

comment:10 Changed 4 months ago by sukhbir

actually using an esr version of the langpacks

https://github.com/arthuredelstein/tor-browser-build/issues/2

comment:11 in reply to:  10 Changed 4 months ago by sukhbir

Replying to sukhbir:

actually using an esr version of the langpacks

https://github.com/arthuredelstein/tor-browser-build/issues/2

Opened a bug, as asked by the developers in #releng:

https://bugzilla.mozilla.org/show_bug.cgi?id=1463749

comment:13 Changed 4 months ago by igt0

Status: newneeds_review

comment:14 Changed 4 months ago by gk

igt0: Could you point me to the branch for this ticket I should review?

comment:15 Changed 4 months ago by sukhbir

https://github.com/azadi/tor-browser-build-1/tree/bug-26073

This branch cherry-picks commits from Arthur's branch but differs by fixing the LLVM signature check (which Arthur just merged.) So you can just merge his branch as most of the changes were lifted from there (and LLVM signature check was merged) but note that it doesn't use master on Tor Launcher and Torbutton, while the one above (bug-26073) does.

This builds on Tor Browser:

+git_hash: '25543+10'
+git_url: https://github.com/arthuredelstein/tor-browser.git

Linux builds fine but Windows fails with:

configure: processing command line
configure: 
configure: error: Option 'CFLAGS=-mwindows -fstack-protector-all -Wstack-protector --param ssp-buffer-size=4 -fno-strict-overflow -Wno-missing-field-initializers -Wformat -Wformat-security' is not recognized

comment:16 in reply to:  15 Changed 4 months ago by tom

Replying to sukhbir:

Linux builds fine but Windows fails with:

configure: processing command line
configure: 
configure: error: Option 'CFLAGS=-mwindows -fstack-protector-all -Wstack-protector --param ssp-buffer-size=4 -fno-strict-overflow -Wno-missing-field-initializers -Wformat -Wformat-security' is not recognized

https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/build/moz.configure/warnings.configure#109-115

comment:17 Changed 4 months ago by gk

FWIW: It's not unreasonable to assume that the first esr60-based nightly branch will be tor-browser-60.0.1esr-8.0-1, i.e. being based on Firefox 60.0.1 ESR.

Last edited 4 months ago by gk (previous) (diff)

comment:18 in reply to:  15 ; Changed 4 months ago by gk

Status: needs_reviewneeds_revision

Replying to sukhbir:

https://github.com/azadi/tor-browser-build-1/tree/bug-26073

This branch cherry-picks commits from Arthur's branch but differs by fixing the LLVM signature check (which Arthur just merged.) So you can just merge his branch as most of the changes were lifted from there (and LLVM signature check was merged) but note that it doesn't use master on Tor Launcher and Torbutton, while the one above (bug-26073) does.

Some comments while looking a bit on the branch:

1) We want to use 60.0.1esr for our nightly builds, if that means we need to disable the lang pack signature checks for now, then be it so.

2) https://github.com/azadi/tor-browser-build-1/commit/69874135a6550419f95ef39d41a4e3cc5aa5fa02 should be redone. There is no need to use the cat hack. Instead one can do ./mach configure "Here Come All The Configure Args" and proceed as usual.

comment:19 in reply to:  14 Changed 4 months ago by igt0

The branch: https://github.com/igortoliveira/tor-browser-build/commits/26073

Bug 26073: Migrate general.useragent.locale to intl.locale.requested
https://github.com/igortoliveira/tor-browser-build/commit/d4f26208caec5bdec62a0bc41e0836d5bf46f83a

Replying to gk:

igt0: Could you point me to the branch for this ticket I should review?

comment:20 Changed 4 months ago by igt0

Status: needs_revisionneeds_review

comment:21 in reply to:  18 Changed 4 months ago by sukhbir

Replying to gk:

Replying to sukhbir:

https://github.com/azadi/tor-browser-build-1/tree/bug-26073

This branch cherry-picks commits from Arthur's branch but differs by fixing the LLVM signature check (which Arthur just merged.) So you can just merge his branch as most of the changes were lifted from there (and LLVM signature check was merged) but note that it doesn't use master on Tor Launcher and Torbutton, while the one above (bug-26073) does.

Some comments while looking a bit on the branch:

1) We want to use 60.0.1esr for our nightly builds, if that means we need to disable the lang pack signature checks for now, then be it so.

Updated. The branch 25543+15 is based on Firefox 60.0.1esr

2) https://github.com/azadi/tor-browser-build-1/commit/69874135a6550419f95ef39d41a4e3cc5aa5fa02 should be redone. There is no need to use the cat hack. Instead one can do ./mach configure "Here Come All The Configure Args" and proceed as usual.

Fixed.

For review: https://github.com/azadi/tor-browser-build-1/tree/bug-26073

Linux x86_64 builds successfully.

comment:22 Changed 4 months ago by sukhbir

PS: I cherry-picked Igor's commit as well.

EDIT: I guess I should have waited for Tor Browser to build as well. Firefox builds but Tor Browser gives an error. I am guessing this is related to the language packs so I will debug this later.

Last edited 4 months ago by sukhbir (previous) (diff)

comment:23 Changed 4 months ago by sukhbir

Here is the error log when building Tor Browser using the latest commit 60.0.1esr (set -x output):

+ rm -f precomplete
+ python /var/tmp/tmp.VlhW2i4ozE/mar-tools/createprecomplete.py
+ popd
~
+ cd /var/tmp/dist/tor-browser
+ MAR_FILE=tor-browser-linux64-8.0a7_en-US.mar
+ MAR=/var/tmp/tmp.VlhW2i4ozE/mar-tools/mar
+ MBSDIFF=/var/tmp/tmp.VlhW2i4ozE/mar-tools/mbsdiff
+ /var/tmp/tmp.VlhW2i4ozE/mar-tools/make_full_update.sh -q /var/tmp/tmp.lSy2JiVd4y/tor-browser-8.0a7-linux-x86_64-682b20/tor-browser-linux64-8.0a7_en-US.mar /var/tmp/dist/tor-browser/tor-browser_en-US/Browser
/var/tmp/dist/tor-browser/tor-browser_en-US/Browser /var/tmp/dist/tor-browser

comment:24 Changed 4 months ago by gk

Status: needs_reviewneeds_revision

Okay, tor-browser-60.0.1esr-8.0-1 is a thing now. Looking at the commits in your bug-26073 a general thing I noticed, please make sure the commits on bug-26073 are still needed for the final branch. For instance there is no reason for us to merge 71f7fd76779cb4b4a96ac17a80a82e5de64d0924 to master. It's been a WIP commit. Moreover, it's not been correct in the sense that it has been changing much more than just the nightly related branch. You only need to change the git_hash part in

  nightly:
    git_hash: 'tor-browser-[% c("var/firefox_version") %]-[% c("var/torbrowser_branch") %]-1'
    tag_gpg_id: 0
    var:
      torbrowser_update_channel: default

+ firefox_platform_version.

e7b62d616f4a339257c69f2b895802c015b829eb does not need to get merged either. Just add a clean commit starting from master dealing with the lang packs. Etc.

comment:25 in reply to:  24 Changed 4 months ago by gk

Replying to gk:

You only need to change the git_hash part in

  nightly:
    git_hash: 'tor-browser-[% c("var/firefox_version") %]-[% c("var/torbrowser_branch") %]-1'
    tag_gpg_id: 0
    var:
      torbrowser_update_channel: default

+ firefox_platform_version.

FWIW: in this case just adaping firefox_platform_version should be enough. Changing the git_hash for the nightly build is just sometimes needed if we make e.g. a -2 tor-browser branch.

comment:26 in reply to:  22 ; Changed 4 months ago by gk

Replying to sukhbir:

PS: I cherry-picked Igor's commit as well.

EDIT: I guess I should have waited for Tor Browser to build as well. Firefox builds but Tor Browser gives an error. I am guessing this is related to the language packs so I will debug this later.

Actually for some reason (( count++ )) in common.sh breaks now. I've pushed a follow-up commit to tor-browser-60.0.1esr-8.0-1 (b135c59f65dba827b61379a4945251e148c43291) to unbreak us, but we should patch all instances of that and find out what the actual underlying issue is (looking at the log of changes to common.sh did not yield some obvious issue).

comment:27 in reply to:  26 ; Changed 4 months ago by gk

Replying to gk:

Replying to sukhbir:

EDIT: I guess I should have waited for Tor Browser to build as well. Firefox builds but Tor Browser gives an error. I am guessing this is related to the language packs so I will debug this later.

Actually for some reason (( count++ )) in common.sh breaks now. I've pushed a follow-up commit to tor-browser-60.0.1esr-8.0-1 (b135c59f65dba827b61379a4945251e148c43291) to unbreak us, but we should patch all instances of that and find out what the actual underlying issue is (looking at the log of changes to common.sh did not yield some obvious issue).

I opened #26216 for that.

comment:28 Changed 4 months ago by gk

I looked over the remaining commits and here are my notes.

69874135a6550419f95ef39d41a4e3cc5aa5fa02 -- not needed with the idea in commit 81da10daeaa1c35cb78e22006e63f1f86c68a1a8; however now I see as the version string:

"tbb-nightly --with-distribution-id=org.torproject --enable-update-channel=default --enable-bundled-fonts"

I wonder if that's related to this change or to our tor-browser patches...

3bbad8b81fc5f04cc331f01d4b1d895e7e77be16 -- merge that with d386c2d78132546c26b7e2d3d6e8ce9cacd9b216
fe6da6bf59edcdede48b931b27b38c9bb0422877 -- okay for now
4c42450871b9a61e04fb97a5163d0895e71452f9 -- not okay

You guard the rust inclusion in the build script behind

+[% IF c("var/linux") %]
+[% END -%]

but not in the config file. I think we should just want to have it enabled unconditionally under the assumption that building ESR60 on the other platforms is broken anyway until we fix the toolchains.

And please merge that commit with the next one.

57045d88b45de321d8c4a9f749ef2c22084b5a0a -- please merge it with the previous commit as this is rust related as well

e41884c0447cc9c39a466cb5b23ea58dfc48c728 -- not okay

Do we have a lib64 dir for 32bit Linux builds? I somehow doubt that. "$rootdir" is not needed (as seen by the tar command we used for cmake previously.

d386c2d78132546c26b7e2d3d6e8ce9cacd9b216 -- merge that with 3bbad8b81fc5f04cc331f01d4b1d895e7e77be16

9d9cc661b187a81f07d3d3da512a4cbb6abdda6c looks good to me

2e0aee0362bfba5c46f2a097aa3383bc468609dd -- not okay

We switched to the informaction.com URL because 5.1.8.5 is not available on AMO anymore, we should switch back to AMO for the WebExtension version.

6b27579b75dec181399662f9475cddcba6bb4cca -- okay

51309b419d6b645fda682665ae2b4091373d590a -- not okay

Please provide a clean commit for dealing with the lang packs, i.e. not a commit to fix arthur's commit.

d2f372825b0cee09f5087976b1ed53edb38a7aef -- not okay
tor-browser-60.0.1esr-8.0-1 is a thing now.

c6fc477bede9707c95e4807be0039befb28a3ccc -- okay

Let me know if anything of that does not make sense to you.

comment:29 in reply to:  28 ; Changed 4 months ago by gk

Replying to gk:

e41884c0447cc9c39a466cb5b23ea58dfc48c728 -- not okay

Do we have a lib64 dir for 32bit Linux builds? I somehow doubt that. "$rootdir" is not needed (as seen by the tar command we used for cmake previously.

I guess we can test what happens if we get rid of the LD_LIBRARY_PATH export. If nothing explodes it's not needed (anymore) and we can safely omit it.

comment:30 in reply to:  29 ; Changed 4 months ago by gk

Replying to gk:

Replying to gk:

e41884c0447cc9c39a466cb5b23ea58dfc48c728 -- not okay

Do we have a lib64 dir for 32bit Linux builds? I somehow doubt that. "$rootdir" is not needed (as seen by the tar command we used for cmake previously.

I guess we can test what happens if we get rid of the LD_LIBRARY_PATH export. If nothing explodes it's not needed (anymore) and we can safely omit it.

As boklm pointed out, we might just have been lucky in our library usage. It's better to fix LD_LIBRARY_PATH properly given that we are using a non-system GCC.

comment:31 in reply to:  24 ; Changed 4 months ago by sukhbir

Status: needs_revisionneeds_review

Replying to gk:

Okay, tor-browser-60.0.1esr-8.0-1 is a thing now. Looking at the commits in your bug-26073 a general thing I noticed, please make sure the commits on bug-26073 are still needed for the final branch. For instance there is no reason for us to merge 71f7fd76779cb4b4a96ac17a80a82e5de64d0924 to master. It's been a WIP commit. Moreover, it's not been correct in the sense that it has been changing much more than just the nightly related branch. You only need to change the git_hash part in

  nightly:
    git_hash: 'tor-browser-[% c("var/firefox_version") %]-[% c("var/torbrowser_branch") %]-1'
    tag_gpg_id: 0
    var:
      torbrowser_update_channel: default

+ firefox_platform_version.

e7b62d616f4a339257c69f2b895802c015b829eb does not need to get merged either. Just add a clean commit starting from master dealing with the lang packs. Etc.

Thanks for the feedback; I started on a clean branch (I didn't want to use --force on a public repository.) Let me know if another approach is preferred.

For review:

https://github.com/azadi/tor-browser-build-1/tree/bug-26073-rev1

69874135a6550419f95ef39d41a4e3cc5aa5fa02 -- not needed with the idea in commit 81da10daeaa1c35cb78e22006e63f1f86c68a1a8; however now I see as the version string:
"tbb-nightly --with-distribution-id=org.torproject --enable-update-channel=default --enable-bundled-fonts"
I wonder if that's related to this change or to our tor-browser patches...

Isn't the string expected given the configuration options we pass? Or was there something else before we switched to this?

4c42450871b9a61e04fb97a5163d0895e71452f9 -- not okay
You guard the rust inclusion in the build script behind
+[% IF c("var/linux") %]
+[% END -%]
but not in the config file. I think we should just want to have it enabled unconditionally under the assumption that building ESR60 on the other platforms is broken anyway until we fix the toolchains.

I guarded the other related changes as well for now, just to be consistent. If you prefer that we enable it unconditionally, let me know.

e41884c0447cc9c39a466cb5b23ea58dfc48c728 -- not okay
Do we have a lib64 dir for 32bit Linux builds? I somehow doubt that. "$rootdir" is not needed (as seen by the tar command we used for cmake previously.

Updated.

2e0aee0362bfba5c46f2a097aa3383bc468609dd -- not okay
We switched to the informaction.com URL because 5.1.8.5 is not available on AMO anymore, we should switch back to AMO for the WebExtension version.

Updated.

51309b419d6b645fda682665ae2b4091373d590a -- not okay
Please provide a clean commit for dealing with the lang packs, i.e. not a commit to fix arthur's commit.

Done.

d2f372825b0cee09f5087976b1ed53edb38a7aef -- not okay
tor-browser-60.0.1esr-8.0-1 is a thing now.

Done.

comment:32 in reply to:  27 Changed 4 months ago by sukhbir

Replying to gk:

Replying to gk:

Replying to sukhbir:

EDIT: I guess I should have waited for Tor Browser to build as well. Firefox builds but Tor Browser gives an error. I am guessing this is related to the language packs so I will debug this later.

Actually for some reason (( count++ )) in common.sh breaks now. I've pushed a follow-up commit to tor-browser-60.0.1esr-8.0-1 (b135c59f65dba827b61379a4945251e148c43291) to unbreak us, but we should patch all instances of that and find out what the actual underlying issue is (looking at the log of changes to common.sh did not yield some obvious issue).

I opened #26216 for that.

Thanks, interesting catch. To confirm, I could build Linux x86_64 with the above branch submitted for review.

comment:33 in reply to:  30 Changed 4 months ago by gk

Replying to gk:

Replying to gk:

Replying to gk:

e41884c0447cc9c39a466cb5b23ea58dfc48c728 -- not okay

Do we have a lib64 dir for 32bit Linux builds? I somehow doubt that. "$rootdir" is not needed (as seen by the tar command we used for cmake previously.

I guess we can test what happens if we get rid of the LD_LIBRARY_PATH export. If nothing explodes it's not needed (anymore) and we can safely omit it.

As boklm pointed out, we might just have been lucky in our library usage. It's better to fix LD_LIBRARY_PATH properly given that we are using a non-system GCC.

Just for the record: the 32bit llvm build does indeed blow up if one uses lib64 instead of lib.

comment:34 Changed 4 months ago by boklm

Some comments on bug-26073-rev1:

6d3afa069c3904b5319b2cfd5e3f9aca45516ea7 (Use cairgo-gtk3 and provide libgtk-3-dev for Firefox 60 ESR)
did you check if libgtk2.0-dev is still needed? If not, we could remove it.

4dc04f54801b6a535a4935af10d42a939086dae5 (Use a more recent version of gcc)
What is the reason for using a more recent version of gcc to build LLVM? If it is because of the LLVM update in the previous commit (61af9560529c3424e), maybe it should be merged with that commit. Or maybe the llvm update could be separated from the previous commit in a new commit named update LLVM
which would also use the newer gcc in the LLVM build.

If you take the patch from #26165 in your branch, the lines added to projects/llvm/build to use a more recent version of gcc can be replaced by:

[% pc('gcc', 'var/setup', { compiler_tarfile => c('input_files_by_name/gcc') }) %]
# We need a link to our GCC, otherwise the system cc gets used which points to
# /usr/bin/gcc.
ln -s gcc /var/tmp/dist/gcc/bin/cc

comment:35 Changed 4 months ago by gk

Blech, the 32-bit build is still failing in the bundling step. :( It seems again be related to the MAR creation.

comment:36 in reply to:  34 Changed 4 months ago by gk

Replying to boklm:

Some comments on bug-26073-rev1:

6d3afa069c3904b5319b2cfd5e3f9aca45516ea7 (Use cairgo-gtk3 and provide libgtk-3-dev for Firefox 60 ESR)
did you check if libgtk2.0-dev is still needed? If not, we could remove it.

All the packages we specify are still needed. I tried to figure out whether we can omit one or the other but the build is failing always in this case.

4dc04f54801b6a535a4935af10d42a939086dae5 (Use a more recent version of gcc)
What is the reason for using a more recent version of gcc to build LLVM? If it is because of the LLVM update in the previous commit (61af9560529c3424e), maybe it should be merged with that commit. Or maybe the llvm update could be separated from the previous commit in a new commit named update LLVM
which would also use the newer gcc in the LLVM build.

Without the newer GCC the LLVM build is breaking.

If you take the patch from #26165 in your branch, the lines added to projects/llvm/build to use a more recent version of gcc can be replaced by:

[% pc('gcc', 'var/setup', { compiler_tarfile => c('input_files_by_name/gcc') }) %]
# We need a link to our GCC, otherwise the system cc gets used which points to
# /usr/bin/gcc.
ln -s gcc /var/tmp/dist/gcc/bin/cc

comment:37 in reply to:  34 Changed 4 months ago by sukhbir

Replying to boklm:

Some comments on bug-26073-rev1:

6d3afa069c3904b5319b2cfd5e3f9aca45516ea7 (Use cairgo-gtk3 and provide libgtk-3-dev for Firefox 60 ESR)
did you check if libgtk2.0-dev is still needed? If not, we could remove it.

Good point; I checked and it seems like it is required.

 0:37.99 DEBUG: configure:10221: checking if app-specific confvars.sh exists
 0:37.99 DEBUG: configure:10678: checking for gtk+-3.0 >= 3.4.0 gtk+-unix-print-3.0 glib-2.0 gobject-2.0
 0:37.99 DEBUG: configure:10685: checking MOZ_GTK3_CFLAGS
 0:37.99 DEBUG: configure:10690: checking MOZ_GTK3_LIBS
 0:37.99 DEBUG: configure:10764: checking for gtk+-2.0 >= 2.18.0 gtk+-unix-print-2.0 glib-2.0 >= 2.22 gobject-2.0 gdk-x11-2.0
 0:38.00 DEBUG: configure: error: Library requirements (gtk+-2.0 >= 2.18.0 gtk+-unix-print-2.0 glib-2.0 >= 2.22 gobject-2.0 gdk-x11-2.0) not met; consider adjusting the PKG_CONFIG_PATH environment variable if your libraries are in a nonstandard prefix so pkg-config can find them.
 0:38.00 ERROR: old-configure failed
 0:38.04 *** Fix above errors and then restart with\
 0:38.04                "/usr/bin/make -f client.mk build"
 0:38.04 make: *** [configure] Error 1

comment:38 in reply to:  35 Changed 4 months ago by gk

Replying to gk:

Blech, the 32-bit build is still failing in the bundling step. :( It seems again be related to the MAR creation.

Okay, we are good here. For some reason I did not use the branch with my mar script fixup.

comment:39 in reply to:  34 Changed 4 months ago by boklm

Replying to boklm:

If you take the patch from #26165 in your branch, the lines added to projects/llvm/build to use a more recent version of gcc can be replaced by:

[% pc('gcc', 'var/setup', { compiler_tarfile => c('input_files_by_name/gcc') }) %]
# We need a link to our GCC, otherwise the system cc gets used which points to
# /usr/bin/gcc.
ln -s gcc /var/tmp/dist/gcc/bin/cc

Actually we want to disable hardening when building llvm, so this should be:

[% pc('gcc', 'var/setup', { compiler_tarfile => c('input_files_by_name/gcc'),
                            hardened_gcc => 0 }) %]
# We need a link to our GCC, otherwise the system cc gets used which points to
# /usr/bin/gcc.
ln -s gcc /var/tmp/dist/gcc/bin/cc

comment:40 Changed 4 months ago by gk

Status: needs_reviewneeds_revision

Okay, we are close it seems. Here comes feedback to the latest branch (bug-26073-rev1):

61af9560529c3424e5a6494c6703ebde8591eea9 -- not okay

There is no need to have two +[% IF c("var/linux") %] blocks right after each other, please merge them.

I don't think you need to have "$rootdir/" in either case.

4dc04f54801b6a535a4935af10d42a939086dae5 -- not okay

We should pick up boklm's idea.

Please rebase your final branch against master.

comment:41 in reply to:  31 ; Changed 4 months ago by gk

Replying to sukhbir:

69874135a6550419f95ef39d41a4e3cc5aa5fa02 -- not needed with the idea in commit 81da10daeaa1c35cb78e22006e63f1f86c68a1a8; however now I see as the version string:
"tbb-nightly --with-distribution-id=org.torproject --enable-update-channel=default --enable-bundled-fonts"
I wonder if that's related to this change or to our tor-browser patches...

Isn't the string expected given the configuration options we pass? Or was there something else before we switched to this?

Not really expected. Right now after running the configure step as we do in your branch config.status shows something like

    'TOR_BROWSER_VERSION': '"asan-esr60 --with-distribution-id=org.torproject --enable-update-channel=defaut --enable-bundled-fonts"',

while we only want to have "asan-esr60" as the version parts.

comment:42 in reply to:  41 Changed 4 months ago by gk

Replying to gk:

Replying to sukhbir:

69874135a6550419f95ef39d41a4e3cc5aa5fa02 -- not needed with the idea in commit 81da10daeaa1c35cb78e22006e63f1f86c68a1a8; however now I see as the version string:
"tbb-nightly --with-distribution-id=org.torproject --enable-update-channel=default --enable-bundled-fonts"
I wonder if that's related to this change or to our tor-browser patches...

Isn't the string expected given the configuration options we pass? Or was there something else before we switched to this?

Not really expected. Right now after running the configure step as we do in your branch config.status shows something like

    'TOR_BROWSER_VERSION': '"asan-esr60 --with-distribution-id=org.torproject --enable-update-channel=defaut --enable-bundled-fonts"',

while we only want to have "asan-esr60" as the version parts.

Ha, after looking at the mach source code it seems to me that mach configure treats the options as one if they enclosed in quotes. So, doing

./mach configure --with-tor-browser-version=[% c("var/torbrowser_version") %] --with-distribution-id=org.torproject --enable-update-channel=[% c("var/torbrowser_update_channel") %] --enable-bundled-fonts

instead should fix this issue...

comment:43 in reply to:  40 Changed 4 months ago by sukhbir

Replying to gk:

Okay, we are close it seems. Here comes feedback to the latest branch (bug-26073-rev1):

61af9560529c3424e5a6494c6703ebde8591eea9 -- not okay

There is no need to have two +[% IF c("var/linux") %] blocks right after each other, please merge them.

I don't think you need to have "$rootdir/" in either case.

4dc04f54801b6a535a4935af10d42a939086dae5 -- not okay

We should pick up boklm's idea.

Please rebase your final branch against master.

For review: https://github.com/azadi/tor-browser-build-1/tree/bug-26073-rev2

This branch is based on master and also addresses the mach comment in #comment:42 in addition to boklm's changes.

comment:44 Changed 4 months ago by sukhbir

Status: needs_revisionneeds_review

comment:45 Changed 4 months ago by gk

Status: needs_reviewneeds_revision

In Firefox for Windows we have

  [% pc('gcc', 'var/setup', { compiler_tarfile => c('input_files_by_name/gcc') }) %]
  # We need a link to our GCC, otherwise the system cc gets used which points to
  # /usr/bin/gcc.
  ln -s gcc /var/tmp/dist/gcc/bin/cc

It seems to me that minimalistic part should work for llvm as well, as all the boilerplate is done in gcc's setup part (with hardened_gcc => 0 as you did).

As I said, if you include var/compiler only for Linux, please make sure this happens in the llvm build script as well.

comment:46 Changed 4 months ago by sukhbir

Status: needs_revisionneeds_review

OK, I updated the branch (sorry, --force this time). See a0a83d9d92eb2a9f20888645a73712584870705a.

comment:47 Changed 4 months ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Resolution: fixed
Status: needs_reviewclosed

Thanks, it seems we are done here now \o/. I merged the following commits to master:

a4ed53bb203f03b76853f42efc0564a75cc7e562
a80e0d5b49e827b613b624c5516b087b3c402f73
74e8c6a9803cc778b73dfa08c23ab7c75bb40718
ed1e40688ca502e1adc1819024c430c24aebebb0
d3339fe091e96daa6fb1918880a3eef82c068cad
b5a67ed318c316c1c5258e2f5e793abbe8bf84ee
77b52782bc81b00a7cd702fa50459cd56a0752e8
66c38da52239e4d8c408f793ce6178f4131cf0cb
4398ed2d7050c8539068d0e6e3ed4a3ca7ff93a7
21b610639107c66bcd49cd219c26560261257aa4
473342eefe9e19dde3a7548b16b29fcfc1f13855.

Closing.

Note: See TracTickets for help on using tickets.