ui: remove generated files from version control

[benesch]

Ignore the first commit; that's #18262.

From the last commit message:

Checking in src/js/protos.js and embedded.go causes several major
problems:

  * Every change to the UI, even if the diff is only one line,
    entirely rewrites embedded.go, which is upwards of 12MB in size.
    This adds a lot of weight to the Git repository.

  * Every commit that touches the UI or protobuf definitions must be
    serialized, as diffs in protos.js and embedded.go cannot be
    automatically merged.

  * Including embedded.go in a pull request breaks GitHub's "request
    review" feature.

Removing these files from version control solves all these problems with
only two downsides:

  * Every developer will need Node and Yarn installed. This seems
    reasonable to expect of our engineers and external contributors.
    This commit teaches our source archives to include a pre-generated
    embedded.go so that end users who simply want to build from source
    without Node and Yarn can do so.

  * Developers will likely need to rebuild the UI a few times per day
    as they switch between branches that span UI changes. This commit
    brings the UI compilation time down to about 5s when only
    first-party sources have been updated and 30s when UI dependencies
    are bumped (infrequently), so this also seems reasonable.

Closes #18349.
Closes #18735.

[bdarnell]

This approach SGTM but don't forget to update CONTRIBUTING.md (and anywhere else we list build requirements).

[cockroach-teamcity]

This change is 

[benesch]

@bdarnell good point, done.

[BramGruneir]

We should get someone who develops on linux to ensure this works. @knz or perhaps @m-schneider. Can one of you see if this works?

---

Reviewed 1 of 6 files at r2, 14 of 18 files at r4, 5 of 6 files at r5, 1 of 1 files at r6.
Review status: 14 of 20 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

CONTRIBUTING.md, line 26 at r6 (raw file):

   - CMake 3.1+
   - Autoconf 2.68+
   - NodeJS 6+ and Yarn 0.22.0+.

Might as well bump yarn to 1.0 +

---

pkg/ui/Makefile, line 38 at r6 (raw file):

	yarn run -- tslint -c tslint.json -p tsconfig.json --type-check
	@# TODO(benesch): Invoke tslint just once when palantir/tslint#2827 is fixed.
	yarn run -- tslint -c tslint.json *.js

For all of these run commands, I'm getting a yarn warning that the -- is being depreciated. Perhaps update your version of yarn?

Can we check at the start of make that node --version > 6 and yarn --version > 1.0 ? This would make things clearer for those you don't RTFM.

---

pkg/ui/tslint.json, line 119 at r6 (raw file):

    ]
  },
  "jsRules": {

Does our JS pass with these rules? Do these rules matter, since all of our JS is generated?

---

pkg/ui/webpack.vendor.js, line 5 at r6 (raw file):

const path = require("path");
const webpack = require("webpack");

nit: extra blank line

---

Comments from Reviewable

[benesch]

Review status: 14 of 20 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

CONTRIBUTING.md, line 26 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Might as well bump yarn to 1.0 +

Hmm, ok. Done.

---

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

For all of these run commands, I'm getting a yarn warning that the -- is being depreciated. Perhaps update your version of yarn?

Can we check at the start of make that node --version > 6 and yarn --version > 1.0 ? This would make things clearer for those you don't RTFM.

time yarn --version
1.0.1

real	0m0.569s

We definitely don't want to check at the beginning of Make, as it'll slow down no-op compiles by .5s. Let me see if I can come up with something else.

---

pkg/ui/tslint.json, line 119 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Does our JS pass with these rules? Do these rules matter, since all of our JS is generated?

Heh, not all of our JS is generated. Specifically, not our JS config files. P.S. this commit is in a standalone PR that's ready to merge after review: #18262

---

pkg/ui/webpack.vendor.js, line 5 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

nit: extra blank line

Intentional separation of first-party and third-party imports, though I guess I'm not sure we actually settled on that as our style moving forward.

---

Comments from Reviewable

[BramGruneir]

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

time yarn --version
1.0.1

real	0m0.569s

We definitely don't want to check at the beginning of Make, as it'll slow down no-op compiles by .5s. Let me see if I can come up with something else.

why would checking a version take so bloody long? That's nuts.

---

Comments from Reviewable

[BramGruneir]

pkg/ui/webpack.vendor.js, line 5 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Intentional separation of first-party and third-party imports, though I guess I'm not sure we actually settled on that as our style moving forward.

Ah, I didn't notice. carry on!

---

Comments from Reviewable

[bdarnell]

Review status: 14 of 20 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

why would checking a version take so bloody long? That's nuts.

Yarn 1.0 is 3 days old. It's reasonable to require non-JS developers to have node installed, but if this is going to require frequent environment updates then we're going to have to rethink this change. Don't increase minimum versions of tools that we don't vendor just to encourage people to keep up to date; only do it when there is a specific reason. (We don't mandate specific versions of gcc/clang, for example, just something that supports c++11)

---

Comments from Reviewable

[BramGruneir]

Reviewed 1 of 18 files at r4, 6 of 6 files at r7, 6 of 6 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Yarn 1.0 is 3 days old. It's reasonable to require non-JS developers to have node installed, but if this is going to require frequent environment updates then we're going to have to rethink this change. Don't increase minimum versions of tools that we don't vendor just to encourage people to keep up to date; only do it when there is a specific reason. (We don't mandate specific versions of gcc/clang, for example, just something that supports c++11)

I had no idea it was so new. And I agree entirely, but I think moving to a 1.0 release over a 0.2 makes sense. To be fair, the only reason I even noticed this was because of the warnings about the change in the run command semantics.

---

Comments from Reviewable

[benesch]

Review status: 0 of 19 files reviewed at latest revision, 4 unresolved discussions.

---

CONTRIBUTING.md, line 26 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Hmm, ok. Done.

Actually, this will require rebuilding the builder image, which I'm really not looking forward to doing. If you don't mind, I'll leave this as allowing 0.22 for the moment, and save the bump to 1.0 later.

---

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I had no idea it was so new. And I agree entirely, but I think moving to a 1.0 release over a 0.2 makes sense. To be fair, the only reason I even noticed this was because of the warnings about the change in the run command semantics.

Yeah, yarn run -- COMMAND is deprecated in favor of yarn run COMMAND in 1.0. I think we can live with the warnings for now. yarn run is kind of useless anyway, given how slow it is; in another PR I can replace every use with node_modules/.bin/COMMAND for a nice speed boost.

---

Comments from Reviewable

[benesch]

Also, I should mention, I added a commit in here to enforce Yarn/Node versions. The restrictions are loose right now (just what CONTRIBUTING.md dictates), but this gives us a path forward when we need to mandate a version upgrade.

---

Review status: 0 of 19 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[BramGruneir]

Reviewed 1 of 18 files at r4, 19 of 20 files at r10.
Review status: 0 of 19 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

CONTRIBUTING.md, line 26 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Actually, this will require rebuilding the builder image, which I'm really not looking forward to doing. If you don't mind, I'll leave this as allowing 0.22 for the moment, and save the bump to 1.0 later.

Gotcha. Ok, leave it as is.

---

pkg/ui/Makefile, line 38 at r6 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, yarn run -- COMMAND is deprecated in favor of yarn run COMMAND in 1.0. I think we can live with the warnings for now. yarn run is kind of useless anyway, given how slow it is; in another PR I can replace every use with node_modules/.bin/COMMAND for a nice speed boost.

SGTM. Perhaps open an issue for this and the yarn bump?

---

Comments from Reviewable

[knz]

I'm getting the following error over here:

installing jsdoc@^3.4.2
installing minimist@^1.2.0
installing espree@^3.4.1
child_process.js:634
    throw err;
    ^

Error: Command failed: npm --silent install jsdoc@^3.4.2 minimist@^1.2.0 espree@^3.4.1
    at checkExecSyncError (child_process.js:591:13)
    at Object.execSync (child_process.js:631:13)
    at modInstall (/data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/protobufjs/cli/util.js:129:19)
    at Object.exports.setup (/data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/protobufjs/cli/util.js:156:5)
    at Object.<anonymous> (/data/home/kena/src/go/src/github.com/cockroachdb/cockroach/pkg/ui/node_modules/protobufjs/cli/pbjs.js:7:6)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)

[BramGruneir]

Once someone with linux tests this, I'll feel a bit better.

---

Reviewed 3 of 18 files at r13, 1 of 6 files at r14, 1 of 1 files at r15.
Review status: 5 of 19 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[a-robinson]

It's awesome to see this, @benesch!

I'd also point out that developers who rely on the builder image won't need node and yarn installed.

[knz]

The error above is due to a missing npm. Please document that npm is required in the contributor guide. LGTM otherwise (FreeBSD).

[couchand]

---

Reviewed 1 of 18 files at r4, 1 of 18 files at r13, 1 of 18 files at r19, 1 of 20 files at r26, 15 of 20 files at r33, 4 of 6 files at r34, 1 of 1 files at r35, 2 of 2 files at r36.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[mrtracy]

This is LGTM

[bdarnell]

node-run.sh LGTM. This class of issue is news to me, but I think the unix-hater's handbook could use a section on file descriptors vs file descriptions.

---

Review status: 6 of 20 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[benesch]

🎉 thanks for the reviews, everyone! Landing this while it's green.