Makefile,pkg/ui: fix compilation of JS CCL protobufs#26148
Makefile,pkg/ui: fix compilation of JS CCL protobufs#26148craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
Best viewed with w=1: https://github.com/cockroachdb/cockroach/pull/26148/files?w=1 |
mberhault
left a comment
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
The C++ version of protos needs an explicit list, so the different behavior probably calls for a comment.
| interface EncryptionStatusProps { | ||
| store: protos.cockroach.server.serverpb.StoreDetails$Properties; | ||
| } | ||
| import EncryptionStatusProps from "oss/src/views/reports/containers/stores/encryption"; |
There was a problem hiding this comment.
I guess I don't understand how ccl/oss import works. I thought CCL took precedence and the OSS stuff wouldn't even exist.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
7172245 to
bc65006
Compare
|
I just had another problem that seems related, although it isn't specifically mentioned here: I changed serverpb/status.proto, and |
couchand
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
looks like this was accidental
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) |
There was a problem hiding this comment.
i don't think these targets should need yarn.opt.installed, just yarn.installed?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
the shell find here looks suspicious
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
shouldn't these targets point at webpack.app.js now?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
are we not cleaning the dlls anymore?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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*. |
There was a problem hiding this comment.
I actually think this comment is quite helpful.
a8becdb to
1eb2452
Compare
benesch
left a comment
There was a problem hiding this comment.
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 |
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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*. |
|
Ok, I'm feeling pretty good about this. Have you found any more bugs, @couchand? |
|
Looks like in my testing I had forgotten the difference between LGTM |
|
TFTR! bors r=couchand,mberhault |
Merge conflict (retrying...) |
|
bors r=couchand,mberhault |
Build failed |
Release note: None
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
|
bors r=couchand,mberhault |
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>
Build succeeded |
|
Oops, this broke |
In cockroachdb#26148 the make watch task was missed. This updates that to run with the new webpack config. Release note: None
|
On it.
…On Mon, Jun 4, 2018 at 3:08 PM, Andrew Couch ***@***.***> wrote:
Oops, this broke make watch.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#26148 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA15INtAt-43VY-UmYCLHD52xs6p7s57ks5t5YXHgaJpZM4UQmPz>
.
|
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.