Improve `build-ui` Makefile target and fix dependencies #2573

albertteoh · 2020-10-17T09:25:27Z

Which problem is this PR solving?

Resolves Simplify contributing.md #2572

Short description of the changes

Remove build-ui step from CONTRIBUTING.md as it's automated by Makefile.
Add UI asset files in clean target to enable rebuilding of UI assets.
Reinstate the explanation of what build-ui does under the hood into the Makefile under its namesake, which was deleted in Update static UI assets path in contrib doc #2548.
build-ui target dependency should only be used for builds that use -tags ui (remove from docker and build-all-in-one-linux targets).
Make build-ui target idempotent by explicitly checking for the UI assets files' existence. If all expected files exist, do nothing. Otherwise, regenerate UI asset files.

Signed-off-by: albertteoh <[email protected]>

Makefile

albertteoh · 2020-10-17T09:27:50Z

Makefile

+	@echo "Expect UI assets: $(EXPECT_ASSETS)"
+	@echo "Found UI assets: $(FOUND_ASSETS)"
+
+ifeq ($(FOUND_ASSETS), $(EXPECT_ASSETS))


I'm open to suggestions of a better approach to checking if build-ui was previously run.

This is what Make was built for - we can to define the files as dependencies of a target. However, the trick is that the files themselves should also be dependency-tracked. I.e. something like this:

placeholder/gen_assets.go: placeholder/public/index.html esc ... actual/gen_assets.go: jaeger-ui/packages/jaeger-ui/build/index.html esc ... build-ui: placeholder/gen_assets.go actual/gen_assets.go # actually, nothing to do here

But, there are still two problems with this:

ideally, jaeger-ui/packages/jaeger-ui/build/index.html should also be dependent on the actual UI source. This gets pretty complicated to manage.

this set-up always requires a real UI build, a situation I would rather avoid. It should only be required when the binary is compiled with -tags ui.

Ah yes, good suggestion re: make on file dependencies.

ideally, jaeger-ui/packages/jaeger-ui/build/index.html should also be dependent on the actual UI source. This gets pretty complicated to manage.

Could we solve this with the following make target spec? I suspect you are suggesting that there's more to consider that I haven't picked up on:

jaeger-ui/packages/jaeger-ui/build/index.html: cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build

this set-up always requires a real UI build, a situation I would rather avoid.

Forgive me for not understanding and my ignorance; what specifically about this set-up requires a real UI build? I thought yarn build would result in a real UI build?

It should only be required when the binary is compiled with -tags ui.

In this PR, I made sure the build-ui target is only executed for binaries compiled with -tags ui, or have I misunderstood something?

Could we solve this with the following make target spec?

The main idea of Make is to execute targets when dependencies change. When the target itself is a file AND has no dependencies, then once that file exists the command will never be run:

$ cat Makefile x: file file: echo n > file $ make x echo n > file $ make x make: Nothing to be done for `x'.

We could add the build dir to the clean target (which doesn't exist when initing the submodule, also note actual/gen_assets.go isn't checked in so it should be okay to remove). So we'd have the following:

.PHONY: clean clean: rm -rf cover.out .cover/ cover.html lint.log fmt.log \ cmd/query/app/ui/actual/gen_assets.go \ jaeger-ui/packages/jaeger-ui/build .PHONY: build-ui build-ui: cmd/query/app/ui/actual/gen_assets.go cmd/query/app/ui/placeholder/gen_assets.go # Do nothing jaeger-ui/packages/jaeger-ui/build/index.html: cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build cmd/query/app/ui/actual/gen_assets.go: jaeger-ui/packages/jaeger-ui/build/index.html esc -pkg assets -o cmd/query/app/ui/actual/gen_assets.go -prefix jaeger-ui/packages/jaeger-ui/build jaeger-ui/packages/jaeger-ui/build cmd/query/app/ui/placeholder/gen_assets.go: cmd/query/app/ui/placeholder/public/index.html esc -pkg assets -o cmd/query/app/ui/placeholder/gen_assets.go -prefix cmd/query/app/ui/placeholder/public cmd/query/app/ui/placeholder/public

From a fresh clone, I performed the following test:

$ git submodule update --init --recursive $ make build-ui cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build esc -pkg assets -o cmd/query/app/ui/actual/gen_assets.go -prefix jaeger-ui/packages/jaeger-ui/build jaeger-ui/packages/jaeger-ui/build esc -pkg assets -o cmd/query/app/ui/placeholder/gen_assets.go -prefix cmd/query/app/ui/placeholder/public cmd/query/app/ui/placeholder/public $ make build-ui # Do nothing # Trigger a rebuild of UI assets $ make clean rm -rf cover.out .cover/ cover.html lint.log fmt.log \ cmd/query/app/ui/actual/gen_assets.go \ jaeger-ui/packages/jaeger-ui/build $ make build-ui cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build esc -pkg assets -o cmd/query/app/ui/actual/gen_assets.go -prefix jaeger-ui/packages/jaeger-ui/build jaeger-ui/packages/jaeger-ui/build

Sounds good. I would change # Do nothing to # Do nothing. If you need to force a rebuild of UI assets, run make clean.

I don't think you need to include cmd/query/app/ui/actual/gen_assets.go in the clean target, because that file will be rebuilt any time index.html is changed (which is being deleted in clean).

codecov · 2020-10-17T10:03:51Z

Codecov Report

Merging #2573 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2573      +/-   ##
==========================================
+ Coverage   95.32%   95.34%   +0.02%     
==========================================
  Files         208      208              
  Lines        9281     9281              
==========================================
+ Hits         8847     8849       +2     
+ Misses        355      354       -1     
+ Partials       79       78       -1

Impacted Files	Coverage Δ
cmd/query/app/server.go	`90.16% <0.00%> (+1.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd59f13...d611b87. Read the comment docs.

yurishkuro · 2020-10-17T16:45:08Z

Makefile

-	rm -rf cover.out .cover/ cover.html lint.log fmt.log
+	rm -rf cover.out .cover/ cover.html lint.log fmt.log \
+		cmd/query/app/ui/actual/gen_assets.go \
+		cmd/query/app/ui/placeholder/gen_assets.go


this file is checked in, I don't think clean should be removing it.

Signed-off-by: albertteoh <[email protected]>

yurishkuro · 2020-10-20T00:33:02Z

Makefile

+	# packages the assets into a Go file that is `.gitignore`-ed.
+	#
+	# The packaged assets can be enabled by providing a build tag `ui`; for example:
+	# $ go run -tags ui ./cmd/all-in-one/main.go


I think CONTRIBUTING file is a better place for this documentation than a makefile, nobody would look here, especially when reading instructions in CONTRIBUTING

yurishkuro

I compared CI run for this PR (https://travis-ci.org/github/jaegertracing/jaeger/builds/736964904) with recent run on master (https://travis-ci.org/github/jaegertracing/jaeger/builds/735758902). The CROSDOCK steps were a few minutes longer, probably because they are now running yarn to build full UI, a step that we intentionally tried to avoid (see target build-crossdock-ui-placeholder). I think that target needs to be changed to also copy index.html from placeholder dir into jaeger-ui/build

yurishkuro · 2020-10-20T00:49:39Z

btw, please give a different title to this PR, I think changes to makefile are more significant than adding documentation

albertteoh · 2020-10-20T01:54:17Z

btw, please give a different title to this PR, I think changes to makefile are more significant than adding documentation

Done, please let me know if the new title is appropriate or if you have suggestions to improve it.

yurishkuro · 2020-10-20T02:49:35Z

Makefile

 	cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build
+
+cmd/query/app/ui/actual/gen_assets.go: jaeger-ui/packages/jaeger-ui/build/index.html


please add to the top of build-crossdock-ui-placeholder target:

mkdir -p jaeger-ui/packages/jaeger-ui/build/ cp cmd/query/app/ui/placeholder/public/index.html jaeger-ui/packages/jaeger-ui/build/index.html

to avoid running yarn for crossdock CI steps

Signed-off-by: albertteoh <[email protected]>

yurishkuro

Thanks, Albert!

albertteoh added 2 commits October 16, 2020 07:09

Add explicit dep of build-ui with jaeger-query

be71f2b

Signed-off-by: albertteoh <[email protected]>

Add back explanation of build-ui and cleanup Makefile

4a3cb32

Signed-off-by: albertteoh <[email protected]>

albertteoh requested a review from a team as a code owner October 17, 2020 09:25

albertteoh requested a review from joe-elliott October 17, 2020 09:25

albertteoh commented Oct 17, 2020

View reviewed changes

yurishkuro reviewed Oct 17, 2020

View reviewed changes

albertteoh added 2 commits October 18, 2020 21:46

Refactor Makefile to use file targets

0ba8144

Signed-off-by: albertteoh <[email protected]>

Clean build dir

8a96682

Signed-off-by: albertteoh <[email protected]>

yurishkuro reviewed Oct 20, 2020

View reviewed changes

albertteoh changed the title ~~Reinstate UI assets explanation and tidy Makefile~~ Improve build-ui Makefile target and fix dependencies Oct 20, 2020

yurishkuro reviewed Oct 20, 2020

View reviewed changes

albertteoh added 2 commits October 20, 2020 15:09

Add require dependency

4be4e8d

Signed-off-by: albertteoh <[email protected]>

Remove build-ui explanation from Makefile

d611b87

Signed-off-by: albertteoh <[email protected]>

yurishkuro approved these changes Oct 20, 2020

View reviewed changes

yurishkuro merged commit 3d99871 into jaegertracing:master Oct 20, 2020

albertteoh deleted the reinstate-ui-assets-explanation branch October 20, 2020 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `build-ui` Makefile target and fix dependencies #2573

Improve `build-ui` Makefile target and fix dependencies #2573

albertteoh commented Oct 17, 2020

albertteoh Oct 17, 2020

yurishkuro Oct 17, 2020

albertteoh Oct 18, 2020 •

edited

Loading

yurishkuro Oct 18, 2020

albertteoh Oct 18, 2020

yurishkuro Oct 19, 2020

codecov bot commented Oct 17, 2020 •

edited

Loading

yurishkuro Oct 17, 2020

yurishkuro Oct 20, 2020

yurishkuro left a comment

yurishkuro commented Oct 20, 2020

albertteoh commented Oct 20, 2020

yurishkuro Oct 20, 2020

yurishkuro left a comment

		cd jaeger-ui && yarn install --frozen-lockfile && cd packages/jaeger-ui && yarn build

		cmd/query/app/ui/actual/gen_assets.go: jaeger-ui/packages/jaeger-ui/build/index.html

Improve build-ui Makefile target and fix dependencies #2573

Improve build-ui Makefile target and fix dependencies #2573

Conversation

albertteoh commented Oct 17, 2020

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Oct 20, 2020

albertteoh commented Oct 20, 2020

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Improve `build-ui` Makefile target and fix dependencies #2573

Improve `build-ui` Makefile target and fix dependencies #2573

albertteoh Oct 18, 2020 •

edited

Loading

codecov bot commented Oct 17, 2020 •

edited

Loading