build_go_lib for executables?
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)?
Activity
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_libwas 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.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 (moved), 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.)
-
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_postoption, move the instructions to generate the archives there, and then remove thebuildfiles. This will avoid having part of the custom instructions inconfigand an other part inbuild.An other minor thing is the missing indentation after the
IF !c("var/go_lib_no_output").Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201906R deleted, TorBrowserTeam201906 added -
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.
I'm not certain whether the patch in this ticket exposes reproducibility issues, but if it does, #31691 (moved) and #31688 (moved) are the most likely culprits.
I'm not certain whether the patch in this ticket exposes reproducibility issues, but if it does, #31691 (moved) and #31688 (moved) 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 installinstead ofgo buildwill expose build paths, but testing confirms that this isn't a problem.)-
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=debe3ddd8e3967d4b56632a9b11b7cde14e01f16I had to adapt the changes from #28803 (moved) in
projects/obfs4/buildto do them inprojects/obfs4/config.JeremyRand: Could you check whether the patch still looks good for you?
mentioned in issue #31691 (moved)
