Skip to content

Makefile,pkg/ui: fix compilation of JS CCL protobufs#26148

Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom
benesch:ui-ccl-protos
Jun 4, 2018
Merged

Makefile,pkg/ui: fix compilation of JS CCL protobufs#26148
craig[bot] merged 3 commits intocockroachdb:masterfrom
benesch:ui-ccl-protos

Conversation

@benesch
Copy link
Copy Markdown
Contributor

@benesch benesch commented May 28, 2018

Main commit message reproduced below, but there are two smaller commits as well.


PR #26040 introduced a dependency on CCL protobufs in the UI. The build
changes in that PR were slightly buggy; specifically, any protobuf that
was depended upon by both an OSS and CCL protobuf would be included in
the JavaScript bundle twice. This sort of double-inclusion causes subtle
problems, like the deepEqual in a unit test that suddenly started
failing 0.

To ensure only one copy of each protobuf is included, have the OSS and
CCL UI builds depend upon completely independent protobuf DLLs. The OSS
protobuf DLL includes only OSS protobufs, of course, while the CCL
protobuf DLL includes both the OSS and CCL protobufs. The protobuf
compiler, pbjs, takes care of including each dependent protobuf exactly
once.

To avoid creating a webpack configuration for each distribution and
distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js),
use webpack's support for parameterized configs. For consistency,
collapse webpack.oss.js and webpack.ccl.js into a parameterized config
in webpack.app.js.

Additionally, deduplicate the Make rules that compile the CCL JavaScript
and TypeScript protobufs. The OSS and CCL rules are nearly identical, so
they can share a recipe with a little bit of Make magic.

@benesch benesch requested review from a team, mberhault and vilterp May 28, 2018 20:59
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented May 28, 2018

Copy link
Copy Markdown
Contributor

@mberhault mberhault left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

PBTS := $(NODE_RUN) $(UI_ROOT)/node_modules/.bin/pbts

JS_PROTOS_CCL := $(filter %/ccl/storageccl/engineccl/enginepbccl/key_registry.proto %/ccl/storageccl/engineccl/enginepbccl/stats.proto,$(GO_PROTOS))
JS_PROTOS_CCL := $(filter %/ccl/storageccl/engineccl/enginepbccl/stats.proto,$(GO_PROTOS))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The C++ version of protos needs an explicit list, so the different behavior probably calls for a comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

interface EncryptionStatusProps {
store: protos.cockroach.server.serverpb.StoreDetails$Properties;
}
import EncryptionStatusProps from "oss/src/views/reports/containers/stores/encryption";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess I don't understand how ccl/oss import works. I thought CCL took precedence and the OSS stuff wouldn't even exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's kind of confusing. "src/FOO" means "give me the CCL version, if it exists, otherwise the OSS version." You can specifically request the CCL or the OSS version by prefixing with "ccl/" or "oss/", respectively.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My theory is that there's never a good reason to explicitly prefix ccl, and you should only ever prefix oss inside a file of the same name in the CCL code.

@benesch benesch force-pushed the ui-ccl-protos branch 3 times, most recently from 7172245 to bc65006 Compare May 30, 2018 18:09
@bdarnell
Copy link
Copy Markdown
Contributor

I just had another problem that seems related, although it isn't specifically mentioned here: I changed serverpb/status.proto, and src/js/protos.js was getting regenerated, but ccl/src/js/protos.js was not. The ccl version is the one that was actually in use, so my new fields weren't working (except for the first change, which I was building from a cleaner state so both versions had to be built). It looks like there's a missing dependency somewhere to regenerate the CCL protos file (not sure whether it's still an issue with this PR or not)

Copy link
Copy Markdown
Contributor

@couchand couchand left a comment

Choose a reason for hiding this comment

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

The general idea here seems reasonable. I left a few specific questions, and I want to spend some time hammering on this to make sure it works for each configuration/change.

I checked the issue that @bdarnell brought up (which I was running into earlier this week), and unfortunately it doesn't seem to be fixed by these changes. I haven't had a chance to dig into it any more than that.

Makefile Outdated
pkg/ui/dist/%.dll.js pkg/ui/%-manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS) $(UI_PROTOS_CCL)
$(NODE_RUN) -C pkg/ui $(WEBPACK) -p --config webpack.$*.js
# [0]: https://stackoverflow.com/a/3077254/1122351 [1]:
# http://savannah.gnu.org/bugs/?19108
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like this was accidental

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops, thanks

Makefile Outdated
# 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.opt.installed $(UI_PROTOS_OSS)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i don't think these targets should need yarn.opt.installed, just yarn.installed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

d'oh! sorry, bad merge resolution with a conflict of my own making (269f387)

pkg/ui/dist%/bindata.go: pkg/ui/webpack.%.js $(UI_DLLS) $(UI_JS) $(UI_JS_CCL) $(UI_MANIFESTS) $(shell find pkg/ui/ccl pkg/ui/src pkg/ui/styl -type f)
@# TODO(benesch): remove references to embedded.go once sufficient time has passed.
rm -f pkg/ui/embedded.go
pkg/ui/distccl/bindata.go: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) $(UI_JS_CCL) $(shell find pkg/ui/ccl -type f)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the shell find here looks suspicious

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. CCL needs to depend on OSS and CCL. Fixed.

Makefile Outdated
pkg/ui/dist/%.oss.dll.js pkg/ui/%.oss.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.opt.installed $(UI_PROTOS_OSS)
$(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.opt.installed $(UI_PROTOS_CCL)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't these targets point at webpack.app.js now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these targets are for vendor.oss.dll.js, protos.oss.dll.js, and protos.ccl.dll.js. The first depends on webpack.vendor.js and the second and third on webpack.protos.js. I believe it's correct as written! Compilation of the app code is handled below in the rule for bindata.go, and that rule does depend on webpack.app.js.

ui-clean: ## Remove build artifacts.
find pkg/ui/dist* -mindepth 1 -not -name dist*.go -delete
rm -f $(UI_DLLS)
rm -f pkg/ui/*manifest.json
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we not cleaning the dlls anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're handled above in the find ... -delete

interface EncryptionStatusProps {
store: protos.cockroach.server.serverpb.StoreDetails$Properties;
}
import EncryptionStatusProps from "oss/src/views/reports/containers/stores/encryption";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My theory is that there's never a good reason to explicitly prefix ccl, and you should only ever prefix oss inside a file of the same name in the CCL code.

// that only the exact directory is checked. A relative path would follow
// the resolution behavior used by node.js for "node_modules", which checks
// for a "node_modules" child directory located in either the current
// directory *or in any parent directory*.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I actually think this comment is quite helpful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@benesch benesch force-pushed the ui-ccl-protos branch 2 times, most recently from a8becdb to 1eb2452 Compare May 31, 2018 17:09
Copy link
Copy Markdown
Contributor Author

@benesch benesch left a comment

Choose a reason for hiding this comment

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

I checked the issue that @bdarnell brought up (which I was running into earlier this week), and unfortunately it doesn't seem to be fixed by these changes. I haven't had a chance to dig into it any more than that.

It's definitely fixed for me. What's broken (and has always been broken AFAICT) is changing a protobuf that serverpb/status.proto depends on, like gossip/gossip.proto; that won't properly trigger a rebuild of the JS protobufs. I'd prefer to leave that to another PR, though. It's kind of annoying to get that right.

Makefile Outdated
pkg/ui/dist/%.dll.js pkg/ui/%-manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.installed $(UI_PROTOS) $(UI_PROTOS_CCL)
$(NODE_RUN) -C pkg/ui $(WEBPACK) -p --config webpack.$*.js
# [0]: https://stackoverflow.com/a/3077254/1122351 [1]:
# http://savannah.gnu.org/bugs/?19108
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops, thanks

Makefile Outdated
# 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.opt.installed $(UI_PROTOS_OSS)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

d'oh! sorry, bad merge resolution with a conflict of my own making (269f387)

Makefile Outdated
pkg/ui/dist/%.oss.dll.js pkg/ui/%.oss.manifest.json: pkg/ui/webpack.%.js pkg/ui/yarn.opt.installed $(UI_PROTOS_OSS)
$(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.opt.installed $(UI_PROTOS_CCL)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these targets are for vendor.oss.dll.js, protos.oss.dll.js, and protos.ccl.dll.js. The first depends on webpack.vendor.js and the second and third on webpack.protos.js. I believe it's correct as written! Compilation of the app code is handled below in the rule for bindata.go, and that rule does depend on webpack.app.js.

pkg/ui/dist%/bindata.go: pkg/ui/webpack.%.js $(UI_DLLS) $(UI_JS) $(UI_JS_CCL) $(UI_MANIFESTS) $(shell find pkg/ui/ccl pkg/ui/src pkg/ui/styl -type f)
@# TODO(benesch): remove references to embedded.go once sufficient time has passed.
rm -f pkg/ui/embedded.go
pkg/ui/distccl/bindata.go: $(UI_CCL_DLLS) $(UI_CCL_MANIFESTS) $(UI_JS_CCL) $(shell find pkg/ui/ccl -type f)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. CCL needs to depend on OSS and CCL. Fixed.

ui-clean: ## Remove build artifacts.
find pkg/ui/dist* -mindepth 1 -not -name dist*.go -delete
rm -f $(UI_DLLS)
rm -f pkg/ui/*manifest.json
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're handled above in the find ... -delete

// that only the exact directory is checked. A relative path would follow
// the resolution behavior used by node.js for "node_modules", which checks
// for a "node_modules" child directory located in either the current
// directory *or in any parent directory*.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented May 31, 2018

Ok, I'm feeling pretty good about this. Have you found any more bugs, @couchand?

@couchand
Copy link
Copy Markdown
Contributor

couchand commented Jun 4, 2018

Looks like in my testing I had forgotten the difference between pkg/server/status/status.proto and pkg/server/serverpb/status.proto and ran into the other issue you mentioned.

LGTM :shipit:

@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented Jun 4, 2018

TFTR!

bors r=couchand,mberhault

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Merge conflict (retrying...)

@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented Jun 4, 2018

bors r=couchand,mberhault

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Build failed

benesch added 3 commits June 4, 2018 13:50
Only the entrypoint protobufs are necessary; dependent protobufs will be
included automatically.

Release note: None
PR cockroachdb#26040 introduced a dependency on CCL protobufs in the UI. The build
changes in that PR were slightly buggy; specifically, any protobuf that
was depended upon by both an OSS and CCL protobuf would be included in
the JavaScript bundle twice. This sort of double-inclusion causes subtle
problems, like the deepEqual in a unit test that suddenly started
failing [0].

To ensure only one copy of each protobuf is included, have the OSS and
CCL UI builds depend upon completely independent protobuf DLLs. The OSS
protobuf DLL includes only OSS protobufs, of course, while the CCL
protobuf DLL includes both the OSS and CCL protobufs. The protobuf
compiler, pbjs, takes care of including each dependent protobuf exactly
once.

To avoid creating a webpack configuration for each distribution and
distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js),
use webpack's support for parameterized configs. For consistency,
collapse webpack.oss.js and webpack.ccl.js into a parameterized config
in webpack.app.js.

Additionally, deduplicate the Make rules that compile the CCL JavaScript
and TypeScript protobufs. The OSS and CCL rules are nearly identical, so
they can share a recipe with a little bit of Make magic.

[0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452

Release note: None
@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented Jun 4, 2018

bors r=couchand,mberhault

craig bot pushed a commit that referenced this pull request Jun 4, 2018
26148: Makefile,pkg/ui: fix compilation of JS CCL protobufs r=couchand,mberhault a=benesch

Main commit message reproduced below, but there are two smaller commits as well.

---

PR #26040 introduced a dependency on CCL protobufs in the UI. The build
changes in that PR were slightly buggy; specifically, any protobuf that
was depended upon by both an OSS and CCL protobuf would be included in
the JavaScript bundle twice. This sort of double-inclusion causes subtle
problems, like the deepEqual in a unit test that suddenly started
failing [0].

To ensure only one copy of each protobuf is included, have the OSS and
CCL UI builds depend upon completely independent protobuf DLLs. The OSS
protobuf DLL includes only OSS protobufs, of course, while the CCL
protobuf DLL includes both the OSS and CCL protobufs. The protobuf
compiler, pbjs, takes care of including each dependent protobuf exactly
once.

To avoid creating a webpack configuration for each distribution and
distribution (i.e., webpack.protos.ccl.js and webpack.protos.oss.js),
use webpack's support for parameterized configs. For consistency,
collapse webpack.oss.js and webpack.ccl.js into a parameterized config
in webpack.app.js.

Additionally, deduplicate the Make rules that compile the CCL JavaScript
and TypeScript protobufs. The OSS and CCL rules are nearly identical, so
they can share a recipe with a little bit of Make magic.

[0]: https://github.com/cockroachdb/cockroach/pull/26040/files#diff-9c8c2f256576e5704513c79be1173f5cR452

26329: roachpb/client: remove LegacyRangeLookup and DeprecatedRangeLookupRequest r=nvanbenschoten a=nvanbenschoten

Cockroach 2.1 clusters will always support meta2 splits, so the legacy
range lookup approach is never allowable. No node in a 2.1 cluster will
send DeprecatedRangeLookupRequests either, so the code is no longer needed.

The change also removes the `DeprecatedReturnIntents` option on ScanRequests.
This option was a predecessor to the READ_UNCOMMITTED read consistency type.
It was only used in 1.1, so it no longer needs to be supported in 2.1 clusters.

26368: opt: enable distsql_union, fix some issues r=RaduBerinde a=RaduBerinde

#### opt: fix two cases of errors caused by "inner" ORDER BY

When the `ORDER BY` adds columns to the scope, we want to remove these
columns before set operations, and for subqueries.

Release note: None

#### opt: fix set operations crash during exec

When building a set operation, we use the leftScope which might have
an ordering. This ordering must be discarded (it can refer to columns
that have been removed, causing a crash when we try to execbuild the
query).

Release note: None

#### opt: split distsql_union and enable for opt

The opt version of the planner test has some distsql plan differences;
they are mostly due to the optimizer ignoring inner ORDER BY (which
the planner doesn't).

Release note: None


Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Build succeeded

@craig craig bot merged commit 7228a63 into cockroachdb:master Jun 4, 2018
@couchand
Copy link
Copy Markdown
Contributor

couchand commented Jun 4, 2018

Oops, this broke make watch.

couchand added a commit to couchand/cockroach that referenced this pull request Jun 4, 2018
In cockroachdb#26148 the make watch task was missed.  This updates that to run with
the new webpack config.

Release note: None
@benesch
Copy link
Copy Markdown
Contributor Author

benesch commented Jun 4, 2018 via email

craig bot pushed a commit that referenced this pull request Jun 4, 2018
26371: build, ui: fix make watch r=couchand a=couchand

In #26148 the make watch task was missed.  This updates that to run with
the new webpack config.

Release note: None

Co-authored-by: Andrew Couch <andrew@cockroachlabs.com>
@benesch benesch deleted the ui-ccl-protos branch June 6, 2018 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants