[TS] Add Build TS to CI jobs #6524

bjornharrtell · 2021-03-20T17:28:07Z

Attempt to resolve #6526

aardappel · 2021-03-22T18:15:33Z

LGTM other than the error, thanks for adding this!

bjornharrtell · 2021-03-22T20:23:21Z

This now also includes a general clean up and removal of duplicate TS code gen artifacts. We have duplicate files here:

https://github.com/google/flatbuffers/tree/master/tests/my-game
https://github.com/google/flatbuffers/tree/master/tests/ts/my-game

I've opted to make sure all code gen ends up in the ts folder, because we have root level files that would end up in the tests folder which is already containing too many files and is not strictly generated content.

I believe this could potentially be merged even if it fails on the new build TS step because it is just a silent failure on master right now.

ping @krojew what is your review and opinion on this?

krojew · 2021-03-23T08:23:12Z

tests/TypeScriptTest.sh

-../flatc --ts --gen-name-strings --gen-mutable --gen-object-api -o ts -I include_test monster_test.fbs
-../flatc --gen-object-api -b -I include_test monster_test.fbs unicode_test.json
-../flatc --ts --gen-name-strings --gen-object-api -o ts union_vector/union_vector.fbs
+if [ -x ../flatc ]; then


I don't think running tests with missing flatc makes much sense - it should rather fail immediately.

The rationale is that we have the artifacts checked in so it's not really a requirement to run the tests, but it's practical when doing local development.

When doing local dev, I don't really see a situation in which someone doesn't build flatc, but wants to run tests. What would be the purpose, since flatc is the thing being worked on?

The argument for not requiring flatc is for the automated CI test run.

Theoretically it can also be useful to be able to do development on https://github.com/google/flatbuffers/tree/master/ts without having to build and regenerate the code gen, but that's probably not important.

We cannot treat those steps as independent - tests have to depend on building, otherwise they might be run before the building process. After all, we're testing the effects of the new build, not a collection of test files from an arbitrary point in time. If the build fails, we cannot test it => invariant is broken.

But that means you consider the whole CI (at GitHub actions) setup wrong and I must disagree. CI passes only if all steps pass so naturally if we cannot build flatc the CI will not pass as a whole. And we do have a step that compiles flatc and explicitly verifies that the resulting code gen is exactly the same as is commited. If we did not commit code gen we would definitely have to do something else, but we commit the code gen for several good reasons.

Notice that we also have a dependency between tests and build, although an implicit one - tests assume some step verifies committed test data comes from the current build. Explicitly stated or not, this is a kind of dependency. If the build output verification step disappears or silently stops working, tests will give wrong result, because their assumption (=dependency) gets broken. They will simply happily test old data.

Don't get me wrong - I'm not blocking merging this PR, but I think we have a problem with our CI configuration if we not only allow implicit dependency on build verification, but we also explicitly allow build to not exist.

Ok I see your point about that the implicit dependency can be seen as too weak or unreliable, though I'm not sure I see it as a big issue.

Anyway I suggest merging this as it is better than nothing and cleans up the duplicate and confusing artifacts we have now.

I am fine either way, but if it needs a tie breaker, I would agree that I prefer whatever setup fails harder/earlier whenever some part is missing, which would suggest @krojew's approach. But again it seems to be a minor difference in this case.

aardappel · 2021-03-25T17:39:30Z

Appveyor fails with:

..\Debug\flatc.exe: error: unknown commandline argument: -ts

which appears to be in generate_code.bat

bjornharrtell · 2021-03-25T19:18:19Z

Unless I'm blind the generate_code.bat is tripping up because the windows version of flatc has some requirement on the input parameters that I fail to understand. :(

aardappel · 2021-03-25T20:23:55Z

The problem is now that the generated code is not the same as that generated by the CI:
https://ci.appveyor.com/project/aardappel/flatbuffers/builds/38406295/job/xsj2ogfk7xc0cj74

bjornharrtell · 2021-03-26T08:50:07Z

For whatever reason I didn't notice earlier that other languages also outputs code gen straight into tests folder, so I've gone back to that. It also exposed issue #6536.

bjornharrtell · 2021-03-26T08:51:01Z

But still seems flatc.exe isn't happy complaining with "..\Debug\flatc.exe: error: invalid option location: -I".

bjornharrtell · 2021-03-26T09:07:16Z

Finally found the problem, I had forgot to fix env var substition to be in the correct format for bat.

This finally now looks be be passing. I've commited the transpiled js files too for not much other reason than that's it's messy to gitignore them and I suppose it doesn't hurt to be able to see the end resulting js in commit diffs.

Note that I've excluded namespace_test from transpilation at https://github.com/bjornharrtell/flatbuffers/blob/ts-build/tests/tsconfig.json#L17 until #6536 can be resolved.

aardappel

whole bunch of files directly in tests, I think we want to avoid that

aardappel · 2021-03-26T18:08:03Z

tests/book-reader.js

@@ -0,0 +1,44 @@
+// automatically generated by the FlatBuffers compiler, do not modify


I don't think these files should be directly in tests, they aren't for any other languages.

aardappel · 2021-03-26T18:10:39Z

Other languages only output to tests if the entirety of the schema compiles to a single file, not for the "one file per type" style that TS uses.

bjornharrtell · 2021-03-26T19:53:02Z

Ok @aardappel I think I could rework to a subfolder.. but even if as you say ts produce more files it's still a mess as is with other langs and I would say that a dedicated code gen folder for all langs would make sense.

aardappel · 2021-03-29T17:17:35Z

we could move more files of other langs to a subfolder as well, but no need to do that here.

bjornharrtell · 2021-03-29T17:54:13Z

@aardappel but so can this be merged as is or?

aardappel · 2021-03-29T18:08:04Z

@bjornharrtell no, I don't think that would be a good idea.. this PR dumps a ton of files in tests which is to be avoided. Like I said, the languages that have files directly in tests like C++ do so because they produce just a single file per schema, which is much less of a problem than the many files TS generates. All other languages have their files under MyGame.

bjornharrtell · 2021-03-29T18:38:35Z

@aardappel ok try to rework it.

bjornharrtell · 2021-03-30T09:14:22Z

@aardappel done and hopefully acceptable now.

bjornharrtell · 2021-04-04T19:18:03Z

Ping @aardappel

tests/ts/rapunzel.ts

tests/ts/optional_scalars/scalar-stuff.ts

bjornharrtell · 2021-04-11T15:06:40Z

Revisited and resolved your observations @aardappel, but now I also realize that the output is not different from other languages and there is no reason that it should have a dedicated ts folder. I will rework that now.

bjornharrtell · 2021-04-11T16:01:12Z

@aardappel this is now again ready. Generated js files are intentionally added with the rationale that upgrades to fx. typescript compiler could affect the output and we want to see the diff changes from such events.

aardappel · 2021-04-12T17:41:54Z

All looks to be in the right spot! Thanks for hanging in there :)

Add Build TS to CI jobs

78edd18

github-actions bot added the CI Continuous Integration label Mar 20, 2021

bjornharrtell added 3 commits March 20, 2021 19:25

Add npm compile step

178a5d0

Fix syntax

7bfa80f

Add required code gen for TS

f9ee1e8

github-actions bot added the typescript label Mar 21, 2021

Exit on failure

3a52765

bjornharrtell changed the title ~~Add Build TS to CI jobs~~ [TS] Add Build TS to CI jobs Mar 21, 2021

bjornharrtell mentioned this pull request Mar 21, 2021

[TS] Error: FlatBuffers: struct must be serialized inline. #6525

Closed

bjornharrtell added 3 commits March 22, 2021 21:13

Make TS code gen identical to test run

9859b6e

Remove duplicate TS code gen artifacts

6f50794

Remove bad gitignore

97df61c

bjornharrtell added 2 commits March 22, 2021 22:49

Forgot parts of generate_code and make sure same settings for test run

374ae1a

Don't forget about the bat

81f7a43

krojew reviewed Mar 23, 2021

View reviewed changes

bjornharrtell added 2 commits March 23, 2021 10:14

Try and fix flatc args

5f6e4f6

Another attempt to fix args

8b2ccc1

bjornharrtell added 5 commits March 25, 2021 19:52

Solve conflict

f790b89

Fix typo

3807431

Commit up to date code gen

fdc571d

Another attempt to fix sh/bat

083d3f5

Move -o param to after -I

e0892e8

bjornharrtell added 2 commits March 26, 2021 00:54

Commit missing code gen file

6aa80a7

Fix grpc code gen and test

c8315ed

github-actions bot added the c++ label Mar 26, 2021

Another grpc code gen fix

cceae4c

github-actions bot added the javascript label Mar 26, 2021

Rework to not use ts folder

d592ae1

Fix env vars in bat and make less churn

a76476d

aardappel requested changes Mar 26, 2021

View reviewed changes

bjornharrtell added 2 commits March 29, 2021 20:50

Move TS code gen to dedicated folder

38beb38

Fix transpilation output folder and module paths

42c16e3

aardappel reviewed Apr 5, 2021

View reviewed changes

tests/ts/rapunzel.ts Outdated Show resolved Hide resolved

tests/ts/optional_scalars/scalar-stuff.ts Outdated Show resolved Hide resolved

bjornharrtell added 4 commits April 11, 2021 16:39

Fixes to code gen

4a7bb05

Include generated js

8e1648e

Merge branch 'master' into ts-build

9bc3a20

Moved ts code gen

d56a4bd

Remove test ts code gen folder

d95860e

aardappel approved these changes Apr 12, 2021

View reviewed changes

aardappel merged commit 408e4db into google:master Apr 12, 2021

bjornharrtell deleted the ts-build branch April 12, 2021 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TS] Add Build TS to CI jobs #6524

[TS] Add Build TS to CI jobs #6524

bjornharrtell commented Mar 20, 2021 •

edited

Loading

aardappel commented Mar 22, 2021

bjornharrtell commented Mar 22, 2021

krojew Mar 23, 2021

bjornharrtell Mar 23, 2021

krojew Mar 23, 2021

bjornharrtell Mar 23, 2021

bjornharrtell Mar 23, 2021

krojew Mar 23, 2021

bjornharrtell Mar 23, 2021

krojew Mar 23, 2021

bjornharrtell Mar 23, 2021

aardappel Mar 25, 2021

aardappel commented Mar 25, 2021

bjornharrtell commented Mar 25, 2021

aardappel commented Mar 25, 2021

bjornharrtell commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

aardappel left a comment

aardappel Mar 26, 2021

aardappel commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

aardappel commented Mar 29, 2021

bjornharrtell commented Mar 29, 2021

aardappel commented Mar 29, 2021

bjornharrtell commented Mar 29, 2021

bjornharrtell commented Mar 30, 2021

bjornharrtell commented Apr 4, 2021

bjornharrtell commented Apr 11, 2021

bjornharrtell commented Apr 11, 2021

aardappel commented Apr 12, 2021

		@@ -0,0 +1,44 @@
		// automatically generated by the FlatBuffers compiler, do not modify

[TS] Add Build TS to CI jobs #6524

[TS] Add Build TS to CI jobs #6524

Conversation

bjornharrtell commented Mar 20, 2021 • edited Loading

aardappel commented Mar 22, 2021

bjornharrtell commented Mar 22, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented Mar 25, 2021

bjornharrtell commented Mar 25, 2021

aardappel commented Mar 25, 2021

bjornharrtell commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

aardappel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aardappel commented Mar 26, 2021

bjornharrtell commented Mar 26, 2021

aardappel commented Mar 29, 2021

bjornharrtell commented Mar 29, 2021

aardappel commented Mar 29, 2021

bjornharrtell commented Mar 29, 2021

bjornharrtell commented Mar 30, 2021

bjornharrtell commented Apr 4, 2021

bjornharrtell commented Apr 11, 2021

bjornharrtell commented Apr 11, 2021

aardappel commented Apr 12, 2021

bjornharrtell commented Mar 20, 2021 •

edited

Loading