Opened 7 weeks ago

Closed 6 weeks ago

Last modified 5 weeks ago

#22831 closed task (fixed)

Merge Snowflake for mac

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

Description

At comment:32:ticket:19001, we have a build of webrtc and snowflake for mac that seems to be reproducible. See also https://lists.torproject.org/pipermail/tbb-dev/2017-July/000579.html.

I'll make a clean merge branch.

Child Tickets

Change History (10)

comment:1 Changed 7 weeks ago by dcf

Keywords: TorBrowserTeam201707R added
Status: newneeds_review

See the snowflake-mac-4 branch.

It's split into four commits. Here is the overall diff: https://gitweb.torproject.org/user/dcf/tor-browser-bundle.git/diff/?h=snowflake-mac-4&id=80b8e83b0d6ae349df4bcaeba34d6002e1bc7ea3&id2=3f466124baf2718906e330b45388c22b2f1a44a7

Here are points you may want to pay special attention to while reviewing (you may know better ways to do them):

comment:3 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201707R removed
Status: needs_reviewneeds_revision

Looks good. The needs_revision is mainly for the following nits:

s/clang 0.3.8/clang 3.8.0/

https://github.com/golang/go/issues/9206#issuecomment-310476743 is used for both path
and timestamp issue. It seems for the latter we want a different URL?

s/The linux descriptor builds its own copy of gn here/The linux descriptor builds its own copy of gn/

s/the gn so build/the gn so built/

---

One thing I was wondering is whether it would be more beneficial to split up the big webrtc patch into
smaller ones. It might make it easier to follow the upstreaming efforts that way giving a clear impression on how many patches still need to get upstreamed and making patches easier to write once one or another of those patches is not needed anymore. I don't feel strongly about that, though. If you want to leave that as-is, fine with me.

I have trouble to understand why faketime is needed now when building go given that we use go built without it for obfs4proxy and meek without any issues. "including those that arise here while compiling the Go runtime itself" should hold for the latter PTs as well, yet we don't need faketime in those cases. What is different between the snowflake-client and, say, the obfs4proxy case so that we need faketime for building go itself now? Are the problematic .o files plainly missing in obfs4proxy's and meeks case? Or...

comment:4 in reply to:  3 Changed 6 weeks ago by dcf

Replying to gk:

Looks good. The needs_revision is mainly for the following nits:

s/clang 0.3.8/clang 3.8.0/

s/The linux descriptor builds its own copy of gn here/The linux descriptor builds its own copy of gn/

s/the gn so build/the gn so built/

Made a snowflake-mac-6 with those changes.

https://github.com/golang/go/issues/9206#issuecomment-310476743 is used for both path
and timestamp issue. It seems for the latter we want a different URL?

That one URL illustrates both the timestamp and the path issues. It only talks about the path issue in the text, though. I couldn't find a more specific link for the timestamp issue. This part of the diff is due to a timestamp:

< 00333430  d4 15 4c 59 00 00 00 00  58 03 00 00 26 03 00 00  |..LY....X...&...|
---
> 00333430  dc 15 4c 59 00 00 00 00  58 03 00 00 26 03 00 00  |..LY....X...&...|

This part of the diff is due to a path:

< 00359fe0  2f 67 6f 2d 6c 69 6e 6b  2d 34 34 37 31 39 35 39  |/go-link-4471959|
< 00359ff0  38 34 2f 67 6f 2e 6f 00  72 75 6e 74 69 6d 65 2e  |84/go.o.runtime.|
---
> 00359fe0  2f 67 6f 2d 6c 69 6e 6b  2d 32 30 38 38 31 36 34  |/go-link-2088164|
> 00359ff0  38 36 2f 67 6f 2e 6f 00  72 75 6e 74 69 6d 65 2e  |86/go.o.runtime.|

One thing I was wondering is whether it would be more beneficial to split up the big webrtc patch into
smaller ones. It might make it easier to follow the upstreaming efforts that way giving a clear impression on how many patches still need to get upstreamed and making patches easier to write once one or another of those patches is not needed anymore. I don't feel strongly about that, though. If you want to leave that as-is, fine with me.

The patch actually is already split up into 8 smaller patches, each with their own commit message. They are just concatenated together into one file (i.e., in git am format).

I have trouble to understand why faketime is needed now when building go given that we use go built without it for obfs4proxy and meek without any issues. "including those that arise here while compiling the Go runtime itself" should hold for the latter PTs as well, yet we don't need faketime in those cases. What is different between the snowflake-client and, say, the obfs4proxy case so that we need faketime for building go itself now? Are the problematic .o files plainly missing in obfs4proxy's and meeks case? Or...

I wondered about that too. I believe it is because of cgo. cgo invokes the system linker (i.e. Clang) and it is actually Clang that is the source of non-determinism, not go itself. See here for example:

When you import "C" in your Go package, go build has to do a lot more work to build your code. Building your package is no longer simply passing a list of all the .go files in scope to a single invocation of go tool compile, instead:

  • The cgo tool needs to be invoked to generate the C to Go and Go to C thunks and stubs.
  • Your system C compiler has to be invoked for every C file in the package.
  • The individual compilation units are combined together into a single .o file.
  • The resulting .o file take a trip through the system linker for fix-ups against shared objects they reference.

comment:5 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Resolution: fixed
Status: needs_revisionclosed

Thanks for the explanations. And, yeah, I meant splitting up the one big patch file in into 8 smaller ones to make it easier to deal with upstreaming etc. But, as I said, I am fine taking that part as-is.

I went forward and picked the remaining commits to master (fcdc2be0a2da32a939e172564300d5a09259b75e, 26e0cd44f2886bfad1c3d30844ff7a21eb9d0478, and ce3fbdf44272e3334d935044599a3b42cf7ff87c).

This will make it into the next alpha.

comment:6 Changed 5 weeks ago by boklm

I opened #22959 for applying the same changes to tor-browser-build.git.

comment:7 Changed 5 weeks ago by boklm

When looking at commit fcdc2be0a2da32a939e172564300d5a09259b75e, I noticed this line in gitian/descriptors/mac/gitian-webrtc.yml:

GN_ARGS+=" gold_path=\"$INSTDIR/binutils/bin\""

However, I don't think we have a binutils installed in this path in the mac build. Is it a line that was copied from the linux descriptor and forgotten?

comment:8 in reply to:  7 ; Changed 5 weeks ago by dcf

Replying to boklm:

When looking at commit fcdc2be0a2da32a939e172564300d5a09259b75e, I noticed this line in gitian/descriptors/mac/gitian-webrtc.yml:

GN_ARGS+=" gold_path=\"$INSTDIR/binutils/bin\""

However, I don't think we have a binutils installed in this path in the mac build. Is it a line that was copied from the linux descriptor and forgotten?

Yes, it is probably a copy-paste error. The GN_ARGS in both descriptors are probably not minimal. The compile–test cycles are so long that I would try changing more than one thing at a time.

If you find that the line is unnecessary, please remove it.

comment:9 in reply to:  8 Changed 5 weeks ago by boklm

Replying to dcf:

Yes, it is probably a copy-paste error. The GN_ARGS in both descriptors are probably not minimal. The compile–test cycles are so long that I would try changing more than one thing at a time.

Ah indeed, testing small changes like this takes a lot of time with gitian. The new build system in tor-browser-build.git should make that kind of changes easier to test as we can rebuild only what changed.

If you find that the line is unnecessary, please remove it.

I did not include this line when converting those changes to tor-browser-build.git, and it seems to work fine without it.

comment:10 Changed 5 weeks ago by gk

Thanks for catching that, boklm. I made a follow-up commit to tor-browser-bundle omitting that line (commit 135c3c89fcc2d2a523d0473a2ac5107bb47bd904).

Note: See TracTickets for help on using tickets.