Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix proto files compilation #664

Merged
merged 1 commit into from
Jan 11, 2017
Merged

fix proto files compilation #664

merged 1 commit into from
Jan 11, 2017

Conversation

ndegory
Copy link
Contributor

@ndegory ndegory commented Jan 10, 2017

fixes #658

The dynamic list for *.pb.gw.go targets was broken, the resulting list contained files that were never generated, triggering systematically the proto compilation, loosing a lot of time.

  • fixing the dynamic proto targets
  • combining the explicit rules
  • removing unneeded lines

How to compare

Timing below are on a Mac, YMMV.
You can notice a big difference in the total cpu time.

Before modification

~/Development/src/github.com/appcelerator/amp(master ✔) make clean
~/Development/src/github.com/appcelerator/amp(master ✔) time make install-cli ; time make install-cli
make install-cli  9.91s user 3.74s system 14% cpu 1:36.78 total
make install-cli  9.45s user 3.98s system 20% cpu 1:07.01 total

After the modification

~/Development/src/github.com/appcelerator/amp(master ✗) make clean
~/Development/src/github.com/appcelerator/amp(master ✗) time make install-cli ; time make install-cli
make install-cli  9.12s user 2.73s system 33% cpu 34.871 total
make install-cli  4.27s user 1.19s system 100% cpu 5.417 total

Copy link
Contributor

@bquenin bquenin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (linux)

@bquenin bquenin merged commit cc8e819 into master Jan 11, 2017
@bquenin bquenin deleted the 658-fix-proto-compilation branch January 11, 2017 10:01
@subfuzion
Copy link
Contributor

@ndegory At some point a while ago, I used *.proto files as the prerequisites for other build rules, the rationale being if if any proto file was updated, then we needed to run protoc on it. At some point it seems that it was removed and I had commented about this previously. I'm just taking a quick cursory look, but it seems that this has gotten more complicated and the dependency is on *.pb.go files, which in reality depend on *.proto files. I want to take a closer look at this, and which rules list depend on these prerequisites. @bquenin As a reminder, I don't want PRs merged without at least two LGTMs on it, and for the time being, one of those being me. Thanks.

@bquenin
Copy link
Contributor

bquenin commented Jan 11, 2017

@subfuzion Sorry about that, I was eager to make compilation time back to normal again.

@ndegory
Copy link
Contributor Author

ndegory commented Jan 11, 2017

@subfuzion please don't blame on Bertrand, I pushed him in merging it. But you're right, I should have waited for 2 LGTMs.

About the dependency, it is restored, there a dynamic variable for the *.proto source files, another for the *.pb.go targets and another for the .pb.gw.go targets. Then there's an explicit rule for the compilation of these targets, and all install- targets depend on these targets plus their own go source files.
So basically, we have something like that:

 *.proto --explicit rule--> *.pb.go-----------
              --explicit rule--> *.pb.gw.go--|
cmd/amp/**.go -------------------------------------> amp CLI target
api/rpc/**.go ---------------------|
vendor/* --------------------------|

subfuzion pushed a commit that referenced this pull request Jan 12, 2017
@subfuzion subfuzion mentioned this pull request Jan 12, 2017
generalhenry pushed a commit that referenced this pull request Jan 12, 2017
* bump version to 0.6.0-dev

* Added amp help command (#652)

* Included Long message in help command output

* fix proto files compilation (#664)

* remove amp-ui (#676)
bquenin pushed a commit that referenced this pull request Jan 12, 2017
* bump version to 0.6.0-dev

* Added amp help command (#652)

* Included Long message in help command output

* fix proto files compilation (#664)

* remove amp-ui (#676)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'make install' takes too long time (more than 30 secs)
3 participants