Opened 3 months ago

Last modified 5 weeks ago

#30334 needs_revision enhancement

build_go_lib for executables?

Reported by: JeremyRand Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, TorBrowserTeam201906
Cc: boklm Actual Points:
Parent ID: Points:
Reviewer: 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 (5)

comment:1 Changed 3 months ago by gk

Cc: boklm added
Keywords: tbb-rbm added

Not sure actually. boklm might know, though.

comment:2 Changed 3 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 weeks 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 weeks ago by gk

Keywords: TorBrowserTeam201906R added
Status: newneeds_review

comment:5 in reply to:  3 Changed 5 weeks 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").

Note: See TracTickets for help on using tickets.