Opened 6 days ago

Closed 3 days ago

Last modified 3 days ago

#28725 closed task (fixed)

Upgrade go-webrtc to dcbfc825aa33471253a5da1834d499257e05d557

Reported by: dcf Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: snowflake tbb-rbm, TorBrowserTeam201812R
Cc: arlolra Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The primary goal of this upgrade was to simplify by removing -D_GLIBCXX_USE_CXX11_ABI=1, which is no longer required in go-webrtc. The upgrade also required upgrades of the dependencies webrtc and depot_tools.

I tested this with make testbuild and running the linux-x86_64 build. I haven't run the linux-i686 and osx-x86_64 builds.

Child Tickets

Attachments (3)

0001-Bug-28725-Upgrade-go-webrtc-to-dcbfc825aa33471253a5d.patch (19.5 KB) - added by dcf 6 days ago.
bug28725_2.patch (20.8 KB) - added by dcf 4 days ago.
bug28725_3.patch (20.9 KB) - added by dcf 4 days ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 days ago by dcf

Status: newneeds_review

comment:2 Changed 6 days ago by gk

Keywords: TorBrowserTeam201812R added

comment:3 Changed 5 days ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201812R removed
Status: needs_reviewneeds_revision

Applying your patch to master and running a build with make testbuild-linux-x86_64 results in a breaking WebRTC build:

[2225/2226] AR obj/ortc/libortc.a 
[2226/2226] AR obj/libwebrtc.a 
cp: cannot stat `./third_party/llvm/tools/clang/test/ARCMT/Inputs/with': No such file or directory 

comment:4 Changed 5 days ago by gk

FWIW: This is reproducible on my machine.

Changed 4 days ago by dcf

Attachment: bug28725_2.patch added

comment:5 Changed 4 days ago by dcf

Keywords: TorBrowserTeam201812R added; TorBrowserTeam201812 removed
Status: needs_revisionneeds_review

Sorry about that. I didn't get any error from a full make testbuild, neither when I originally staged the patch on 3e75d4bbf273c1494ff9442c67c0dda1f86c6eac, nor when I tried rebasing on 30c764db2bbbde93ebee67e2b190a4d9c88873b9 yesterday. I don't even have a directory called gclient/webrtc/src/third_party/llvm; mine is called gclient/webrtc/src/third_party/llvm-build.

Nevertheless, I am guessing from the error message that there is a problem with whitespace in filenames. Here's a revised patch that rewrites the for loops to instead use whitespace-safe find -print0 and xargs -0. This additional patch doesn't affect the output for me; i.e., my corresponding out/webrtc/webrtc-*.tar.gz files are the same except for the gzip timestamp.

comment:6 Changed 4 days ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201812R removed
Status: needs_reviewneeds_revision

Still broken:

[2226/2226] AR obj/libwebrtc.a
dirname: extra operand `space/test.h'
Try `dirname --help' for more information.
cp: cannot create regular file `/var/tmp/dist/webrtc/include/./third_party/llvm/tools/clang/test/ARCMT/Inputs/with space/test.h': No such file or directory

I have both third-party/llvm and third-party/llvm-build for what it's worth. git status run in gclient/webrtc/src/third_party gives

gk@stefan:~/tor-browser-build/gclient/webrtc/src/third_party$ git status
HEAD detached at 6a94dad
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	gtest-parallel/
	llvm-build-tools/
	winsdk_samples/

Does that ring some bell?

Changed 4 days ago by dcf

Attachment: bug28725_3.patch added

comment:7 in reply to:  6 ; Changed 4 days ago by dcf

Status: needs_revisionneeds_review

Here's another. I haven't tested it but the only difference is

-  mkdir -p "$INCLUDE_DIR/$(dirname $h)"
+  mkdir -p "$INCLUDE_DIR/$(dirname "$h")"

Replying to gk:

I have both third-party/llvm and third-party/llvm-build for what it's worth. git status run in gclient/webrtc/src/third_party gives

gk@stefan:~/tor-browser-build/gclient/webrtc/src/third_party$ git status
HEAD detached at 6a94dad
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	gtest-parallel/
	llvm-build-tools/
	winsdk_samples/

Does that ring some bell?

Mine is different:

~/tor-browser-build/gclient/webrtc/src/third_party$ git status
HEAD detached at 6a94dad699
Untracked files:
  (use "git add <file>..." to include in what will be committed)

        gflags/
        gtest-parallel/

/llvm is in gclient/webrtc/src/third_party/.gitignore, so I guess because your checkout is older than mine you have some old leftover files. Since upstream changed from storing all their headers under webrtc/ to storing them all under ./, I suppose some extraneous cruft like that gets included. I think it should be harmless.

comment:8 Changed 4 days ago by gk

Keywords: TorBrowserTeam201812R added; TorBrowserTeam201812 removed

comment:9 in reply to:  7 ; Changed 4 days ago by boklm

Replying to dcf:

/llvm is in gclient/webrtc/src/third_party/.gitignore, so I guess because your checkout is older than mine you have some old leftover files. Since upstream changed from storing all their headers under webrtc/ to storing them all under ./, I suppose some extraneous cruft like that gets included. I think it should be harmless.

I'm wondering if there is something we can do in fetch_sources in projects/webrtc/config to clean those old files (maybe in a separate ticket, if that is not related to this issue).

comment:10 in reply to:  7 Changed 3 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to dcf:

Here's another. I haven't tested it but the only difference is

-  mkdir -p "$INCLUDE_DIR/$(dirname $h)"
+  mkdir -p "$INCLUDE_DIR/$(dirname "$h")"

That looks better better, thanks. I applied both commits to master (commit d8db312e4527c694e8f906a5e8926d0dc0c37179 and 2e1d3917340c980026c15ed9cf10e894ec665551).

comment:11 in reply to:  9 Changed 3 days ago by dcf

Replying to boklm:

I'm wondering if there is something we can do in fetch_sources in projects/webrtc/config to clean those old files (maybe in a separate ticket, if that is not related to this issue).

I'm also wondering if we can tighten up the list of directories from which we pull headers. I may try doing this in the go-webrtc upstream, and file a ticket here if it works.

Note: See TracTickets for help on using tickets.