Opened 8 months ago

Last modified 4 days ago

#30334 needs_review enhancement

build_go_lib for executables?

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

Description

Is there a reason that build_go_lib (from the go project in tor-browser-build) is only used for libraries and not executables? At first glance, it seems to me that using it (or something very similar) for executables as well would cut down on boilerplate / code duplication. Would a patch be accepted that adapted the meek/obfs4/snowflake projects in tor-browser-build to use build_go_lib (or an executable-focused analogue of it)?

Child Tickets

Change History (21)

comment:1 Changed 8 months ago by gk

Cc: boklm added
Keywords: tbb-rbm added

Not sure actually. boklm might know, though.

comment:2 Changed 7 months ago by boklm

Would a patch be accepted that adapted the meek/obfs4/snowflake projects in tor-browser-build to use build_go_lib (or an executable-focused analogue of it)?

I think yes, if it allows to cut down on boilerplate / code duplication and does not make things more complex.

The reason I added build_go_lib was that all the go libraries are built in the same way, with little differences, so having some instructions that works for all of them was easy. For meek/obfs4/snowflake, there are lot more differences, so it's more difficult to do, and less clear whether it is worth it.

comment:3 Changed 6 months ago by JeremyRand

Patch at https://notabug.org/JeremyRand/tor-browser-build/src/build-go-lib-exe (commit hash 690a8334a7c7c3e7db40f09783da7096d5ab4c56). There was indeed a lot of boilerplate / duplicated code present in meek/obfs4/snowflake, and I think this patch does a reasonably good job of improving the situation. I definitely find the code easier to read with this patch applied, though admittedly this is subjective and I'm probably biased toward finding code that I wrote easy to read.

Feel free to review. I've tested all of the build targets that I could think of to make sure that it builds without errors and that the outputs of meek/obfs4/snowflake look sane when untarred, but I haven't tested the resulting Tor Browser binaries.

As a side note, this patch would make my life easier with regards to #30558, since solving that ticket involves adding another Go executable, which exacerbates the code duplication issues unless this patch is applied. (I'm not familiar enough with Trac culture to know how to tag that relationship, or whether I even have the needed privs to do so.)

comment:4 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906R added
Status: newneeds_review

comment:5 in reply to:  3 Changed 6 months ago by boklm

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201906R removed
Status: needs_reviewneeds_revision

Replying to JeremyRand:

Patch at https://notabug.org/JeremyRand/tor-browser-build/src/build-go-lib-exe (commit hash 690a8334a7c7c3e7db40f09783da7096d5ab4c56). There was indeed a lot of boilerplate / duplicated code present in meek/obfs4/snowflake, and I think this patch does a reasonably good job of improving the situation. I definitely find the code easier to read with this patch applied, though admittedly this is subjective and I'm probably biased toward finding code that I wrote easy to read.

I think this looks mostly good.

A possible improvement is to add a var/build_go_lib_post option, move the instructions to generate the archives there, and then remove the build files. This will avoid having part of the custom instructions in config and an other part in build.

An other minor thing is the missing indentation after the IF !c("var/go_lib_no_output").

comment:6 Changed 5 months ago by JeremyRand

Thanks for the review @boklm. I've made the requested changes, Git commit hash 07fc3310bcee2d39def889b48c2d7120d4b96e48. To make it easier to review the new changes, I didn't yet squash the commits; let me know if you'd like me to squash them prior to a merge.

comment:7 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201906 removed
Status: needs_revisionneeds_review

comment:8 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:9 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201908R removed

No August anymore.

comment:10 Changed 2 months ago by pili

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201909R removed

We're now in October, moving September outstanding reviews to October

comment:11 Changed 6 weeks ago by pili

Keywords: TorBrowserTeam201911 added

Moving tickets to November 2019

comment:12 in reply to:  6 Changed 6 weeks ago by boklm

Replying to JeremyRand:

Thanks for the review @boklm. I've made the requested changes, Git commit hash 07fc3310bcee2d39def889b48c2d7120d4b96e48. To make it easier to review the new changes, I didn't yet squash the commits; let me know if you'd like me to squash them prior to a merge.

I rebased/adapted the patch on master:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30334&id=ca460ed70d8726ae1c6805ab1e870d230ded951e

However I did not check yet that it still builds correctly.

comment:13 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201910R removed

There is no way to do reviews in October 2019 anymore.

comment:14 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201911 removed

No need for duplicate keyword.

comment:15 Changed 4 weeks ago by pili

Cc: tbb-team added
Owner: changed from tbb-team to boklm
Status: needs_reviewassigned

Assigning tickets to boklm for the next few months

comment:16 Changed 4 weeks ago by JeremyRand

I'm not certain whether the patch in this ticket exposes reproducibility issues, but if it does, #31691 and #31688 are the most likely culprits.

comment:17 Changed 4 weeks ago by JeremyRand

I'm not certain whether the patch in this ticket exposes reproducibility issues, but if it does, #31691 and #31688 are the most likely culprits.

Further investigation suggests that the changes in this ticket's patch won't expose any new reproducibility issues. (I was wondering if the usage of go install instead of go build will expose build paths, but testing confirms that this isn't a problem.)

comment:18 Changed 4 weeks ago by sysrqb

Status: assignedneeds_review

comment:19 Changed 12 days ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:20 Changed 12 days ago by pili

Reviewer: boklm

boklm to review

comment:21 Changed 4 days ago by boklm

I rebased the patch on master in branch bug_30334_v2:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_30334_v2&id=debe3ddd8e3967d4b56632a9b11b7cde14e01f16

I had to adapt the changes from #28803 in projects/obfs4/build to do them in projects/obfs4/config.

JeremyRand: Could you check whether the patch still looks good for you?

Note: See TracTickets for help on using tickets.