Skip to content

Commit

Permalink
Merge #68038 #68059
Browse files Browse the repository at this point in the history
68038: build: remove pointless rebuilds of cluster-ui r=jordanlewis a=jordanlewis

Since 60916fb, any change to any file in any subdirectory of pkg/ui
would cause a full rebuild of pkg/ui/cluster-ui, which would then cause
a full rebuild of pkg/ui as well.

Now, pkg/ui/cluster-ui's source files are tracked separately, so changes
to pkg/ui/ that don't touch cluster-ui won't force a rebuild of
cluster-ui.

This should save a lot of time, hopefully. Touching a single .tsx file
in pkg/ui/src used to cause 30+ seconds of build time due to this issue,
now it is significantly faster since the various webpack caches can
actually be used again.

Release note: None

cc @cockroachdb/cluster-ui-prs @cockroachdb/sql-observability 

68059: sql: skip TestCancelQueryPermissions r=irfansharif a=tbg

Refs: #63547

Reason: https://cockroachlabs.slack.com/archives/C016CAD2HQ8/p1627290710022700

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
  • Loading branch information
3 people committed Jul 26, 2021
3 parents 3fd1428 + d4d1b14 + d6e07ae commit 225234e
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 4 deletions.
15 changes: 11 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,6 @@ pkg/ui/yarn.cluster-ui.installed: pkg/ui/cluster-ui/package.json pkg/ui/cluster-
$(NODE_RUN) -C pkg/ui/cluster-ui yarn install --offline
# This ensures that any update to the protobuf will be picked up by cluster-ui.
$(NODE_RUN) -C pkg/ui/cluster-ui yarn upgrade file:pkg/ui/src/js
$(NODE_RUN) -C pkg/ui/cluster-ui yarn build
touch $@

.SECONDARY: pkg/ui/yarn.protobuf.installed
Expand Down Expand Up @@ -1345,6 +1344,15 @@ ui-lint: pkg/ui/yarn.installed $(UI_PROTOS_OSS) $(UI_PROTOS_CCL)
@if $(NODE_RUN) -C pkg/ui yarn list | grep phantomjs; then echo ^ forbidden UI dependency >&2; exit 1; fi
$(NODE_RUN) -C pkg/ui/cluster-ui yarn --cwd pkg/ui/cluster-ui lint

CLUSTER_UI_JS := pkg/ui/cluster-ui/dist/main.js

.SECONDARY: $(CLUSTER_UI_JS)

# The grep -v " " is to exclude a couple of font files that have spaces in their filenames, which
# Make is unable to deal with as far as I can tell.
$(CLUSTER_UI_JS): pkg/ui/yarn.installed $(shell find pkg/ui/cluster-ui/src -type f | grep -v " ")
$(NODE_RUN) -C pkg/ui/cluster-ui yarn build

# DLLs are Webpack bundles, not Windows shared libraries. See "DLLs for speedy
# builds" in the UI README for details.
UI_CCL_DLLS := pkg/ui/dist/protos.ccl.dll.js pkg/ui/dist/vendor.oss.dll.js
Expand All @@ -1368,10 +1376,10 @@ UI_OSS_MANIFESTS := $(subst .ccl,.oss,$(UI_CCL_MANIFESTS))
# [1]: http://savannah.gnu.org/bugs/?19108
.SECONDARY: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) $(UI_OSS_DLLS) $(UI_OSS_MANIFESTS)

pkg/ui/dist/%.oss.dll.js pkg/ui/%.oss.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS_OSS)
pkg/ui/dist/%.oss.dll.js pkg/ui/%.oss.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS_OSS) $(CLUSTER_UI_JS)
$(NODE_RUN) -C pkg/ui $(WEBPACK) -p --config webpack.$*.js --env.dist=oss

pkg/ui/dist/%.ccl.dll.js pkg/ui/%.ccl.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS_CCL)
pkg/ui/dist/%.ccl.dll.js pkg/ui/%.ccl.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS_CCL) $(CLUSTER_UI_JS)
$(NODE_RUN) -C pkg/ui $(WEBPACK) -p --config webpack.$*.js --env.dist=ccl

.PHONY: ui-test
Expand All @@ -1395,7 +1403,6 @@ pkg/ui/dist%/bindata.go: pkg/ui/webpack.app.js $(shell find pkg/ui/src pkg/ui/st
set -e; shopt -s extglob; for dll in $(notdir $(filter %.dll.js,$^)); do \
ln -s ../dist/$$dll pkg/ui/dist$*/$${dll/@(.ccl|.oss)}; \
done
$(NODE_RUN) -C pkg/ui/cluster-ui yarn build
$(NODE_RUN) -C pkg/ui $(WEBPACK) --config webpack.app.js --env.dist=$*
go-bindata -pkg dist$* -o $@ -prefix pkg/ui/dist$* pkg/ui/dist$*/...
echo 'func init() { ui.Asset = Asset; ui.AssetDir = AssetDir; ui.AssetInfo = AssetInfo }' >> $@
Expand Down
1 change: 1 addition & 0 deletions build/variables.mk
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ define VALID_VARS
CGO_SUFFIXED_FLAGS_FILES
CGO_UNSUFFIXED_FLAGS_FILES
CLUSTER
CLUSTER_UI_JS
COCKROACH
COCKROACHOSS
COCKROACHSHORT
Expand Down
2 changes: 2 additions & 0 deletions pkg/sql/run_control_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sqltestutils"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/testutils/skip"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
Expand Down Expand Up @@ -241,6 +242,7 @@ GRANT admin TO has_admin2;

func TestCancelQueryPermissions(t *testing.T) {
defer leaktest.AfterTest(t)()
skip.WithIssue(t, 63547, "https://cockroachlabs.slack.com/archives/C016CAD2HQ8/p1627290710022700")
defer log.Scope(t).Close(t)

// getQueryIDs retrieves the IDs of any currently running queries for the
Expand Down

0 comments on commit 225234e

Please sign in to comment.