ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.86k stars 295 forks source link

Codecgen does not install package dependencies, which could make clean builds slow if codecgen run first #145

Closed 2opremio closed 8 years ago

2opremio commented 8 years ago

We have chosen for our build system to invoke codecgen on the fly instead of committing the resulting files to avoid the problems resulting from it (conflicts etc ...).

However, clean builds are really slow because codecgen, when invoking go run, builds all the dependencies of the package temporarily and throws the result away. After that, we build the project itself, and all that work needs to be repeated, causing a tremendous slowdown in our CI pipeline.

To fix this, I had a patch on the way to pass -i to go run: https://github.com/2opremio/go-1/commit/c99c16dd7787e9a24ca61630968d288bf870b0c6 ... except go run doesn't seem to accept -i.

Using go build+executing the resulting file instead of invoking go run would make this possible, but I wanted to check if this would be acceptable before submitting a patch.

See https://github.com/weaveworks/scope/issues/1004

ugorji commented 8 years ago

The hold up is that codecgen takes a -rt flag, which is the flags to pass to "go run -tags=XYZ". I think that is at odds with a clean build.

Try out some theories first, and let me know how they work:

ugorji commented 8 years ago

will wait on response from @2opremio before moving further

2opremio commented 8 years ago

I tried all the reasonable options before submitting this issue (I temporarily modified codecgen to pass -x to go run and observed the impact of the different options), as a result I suggested the -i solution.

Here's a summary:

Without -rt tags, codecgen invokes go run and rebuilds all the dependencies again, because we provide tags when building packages, and go rebuilds all dependencies when different tags are provided, hence https://github.com/weaveworks/scope/commit/c741ee74d2ce11f0f7947487b7648ad594007458 . Once I added -rt tags, codecgen is fast in working directories where the packages have already been installed.

However, when building from scratch, codecgen builds all the packages and throws them away instead of installing them. Replacing go run by go build -i+exec fixes that problem.

with a possible -i flag, which cannot be set at same time with -rt flag

I am not sure I follow. -tags and -i are independent and can both be used at the same time.

building some pre-requisite packages before running codecgen, so codecgen doesn't have to build things which are already built

This is really inconvenient (an artificial requirement for the build system) and thus it's the only option I haven't tried.

ugorji commented 8 years ago

I am not comfortable with codecgen being in charge of installing packages. There are questions that arise that we don't fully understand e.g.

IMO, codecgen should always do a transient build (like "go run").

To workaround that the time concerns, I think callers should run "go install ... " on dependent packages first, before calling "codecgen". In a global build, that's an easy requirement to swallow (build writers carefully tune build system to be efficient). In a one-off run, there's no problem; users pay the cost.

Let's try to work within the system as is today. If there are challenges there, then we can look into extending the tool to support it.

2opremio commented 8 years ago

I am not comfortable with codecgen being in charge of installing packages. There are questions that arise that we don't fully understand

Even if it's hidden behind a flag?

IMO, codecgen should always do a transient build (like "go run").

Right, but that has pretty bad performance implications

To workaround that the time concerns, I think callers should run "go install ... " on dependent packages first, before calling "codecgen"

This means your users need to pay the price (by modifying the build system) and know the implications of codecgen using go run.

To be frank I think it's probably easier for us to make a fork of codecgen accepting -i than changing/maintaining our build system to build all the dependencies first.

ugorji commented 8 years ago

Understood. I can't stop you from doing a fork. But I would discourage it.

What you are doing is tantamount to:

go build -i
codecgen ...

So why not just do it explicitly if that's what you are going for?

That way, the codecgen tool stays true to its philosophy that it is not a build tool, but uses "go run ... " as an implementation detail, and you are helping improve the performance of that implementation detail. Add -i flag suggests it is a build tool, and that is wrong.

What I wrote above is my recommendation:

Hope this helps.

2opremio commented 8 years ago

What you are doing is tantamount to: ..

Alright, I will give that a try.

ugorji commented 8 years ago

update???

2opremio commented 8 years ago

We ended up doing this, yep. I am not very happy with the workaround though.

ugorji commented 8 years ago

Thanks. Closing now.

2opremio commented 8 years ago

Unfortunately it's not as simple as

go build -i .
codecgen -o output.go file1.go file2.go ...

You also need to make sure output.go is not included in the build see https://github.com/weaveworks/scope/issues/1134